Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eslint plugin: Add suggestions to "data-no-store-string-literals" rule #28974

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,21 @@ const valid = [
`import { controls as controlsAlias } from '@wordpress/data'; import { store as coreStore } from '@wordpress/core-data'; controlsAlias.resolveSelect( store );`,
];

const createSuggestionTestCase = ( code, output ) => ( {
code,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output,
},
],
},
],
} );

const invalid = [
// Callback functions
`import { createRegistrySelector } from '@wordpress/data'; createRegistrySelector(( select ) => { select( 'core' ); });`,
Expand All @@ -57,6 +72,50 @@ const invalid = [
`import { controls } from '@wordpress/data'; controls.dispatch( 'core' );`,
`import { controls } from '@wordpress/data'; controls.resolveSelect( 'core' );`,
`import { controls as controlsAlias } from '@wordpress/data'; controlsAlias.resolveSelect( 'core' );`,

// Direct function calls suggestions
// Replace core with coreStore and import coreStore
createSuggestionTestCase(
`import { select } from '@wordpress/data'; select( 'core' );`,
`import { select } from '@wordpress/data';\nimport { store as coreStore } from '@wordpress/core-data'; select( coreStore );`
),
// Replace core with coreStore. A @wordpress/core-data already exists, so it should append the import to that one.
createSuggestionTestCase(
`import { select } from '@wordpress/data'; import { something } from '@wordpress/core-data'; select( 'core' );`,
`import { select } from '@wordpress/data'; import { something,store as coreStore } from '@wordpress/core-data'; select( coreStore );`
),
// Replace core with coreStore. A @wordpress/core-data already exists, so it should append the import to that one.
// This time there is a comma after the import.
createSuggestionTestCase(
`import { select } from '@wordpress/data'; import { something, } from '@wordpress/core-data'; select( 'core' );`,
`import { select } from '@wordpress/data'; import { something,store as coreStore, } from '@wordpress/core-data'; select( coreStore );`
),
// Replace core with coreStore. Store import already exists. It shouldn't modify the import, just replace the literal with the store definition.
createSuggestionTestCase(
`import { select } from '@wordpress/data'; import { store as coreStore } from '@wordpress/core-data'; select( 'core' );`,
`import { select } from '@wordpress/data'; import { store as coreStore } from '@wordpress/core-data'; select( coreStore );`
),
// Replace core with coreStore. There are internal and WordPress dependencies.
// It should append the import after the last WordPress dependency import.
createSuggestionTestCase(
`import { a } from './a'; import { select } from '@wordpress/data'; import { b } from './b'; select( 'core' );`,
`import { a } from './a'; import { select } from '@wordpress/data';\nimport { store as coreStore } from '@wordpress/core-data'; import { b } from './b'; select( coreStore );`
),
// Replace block-editor with blockEditorStore
createSuggestionTestCase(
`import { select } from '@wordpress/data'; select( 'core/block-editor' );`,
`import { select } from '@wordpress/data';\nimport { store as blockEditorStore } from '@wordpress/block-editor'; select( blockEditorStore );`
),
// Replace notices with noticesStore
createSuggestionTestCase(
`import { select } from '@wordpress/data'; select( 'core/notices' );`,
`import { select } from '@wordpress/data';\nimport { store as noticesStore } from '@wordpress/notices'; select( noticesStore );`
),
// Replace edit-post with editPostStore
createSuggestionTestCase(
`import { select } from '@wordpress/data'; select( 'core/edit-post' );`,
`import { select } from '@wordpress/data';\nimport { store as editPostStore } from '@wordpress/edit-post'; select( editPostStore );`
),
];
const errors = [
{
Expand All @@ -66,5 +125,7 @@ const errors = [

ruleTester.run( 'data-no-store-string-literals', rule, {
valid: valid.map( ( code ) => ( { code } ) ),
invalid: invalid.map( ( code ) => ( { code, errors } ) ),
invalid: invalid.map( ( code ) =>
typeof code === 'string' ? { code, errors } : code
),
} );
107 changes: 107 additions & 0 deletions packages/eslint-plugin/rules/data-no-store-string-literals.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,33 @@
/**
* Converts store name to variable name.
* Removes dashes and uppercases the characters after dashes and appends `Store` at the end.
*
* @param {string} storeName
* @return {string} store name as variable name
*/
function storeNameToVariableNames( storeName ) {
return (
storeName
.split( '-' )
.map( ( s, index ) =>
david-szabo97 marked this conversation as resolved.
Show resolved Hide resolved
index === 0
? s.toLowerCase()
: s[ 0 ].toUpperCase() + s.slice( 1 ).toLowerCase()
)
.join( '' ) + 'Store'
);
}

/**
* Returns last element of an array.
*
* @param {Array} array
* @return {*} last element of the array
*/
function arrayLast( array ) {
return array[ array.length - 1 ];
}

function getReferences( context, specifiers ) {
const variables = specifiers.reduce(
( acc, specifier ) =>
Expand Down Expand Up @@ -85,6 +115,82 @@ function collectAllNodesFromObjectPropertyFunctionCalls( context, node ) {
return possibleCallExpressionNodes;
}

function getSuggest( context, callNode ) {
return [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
fix: ( fixer ) => getFixes( fixer, context, callNode ),
},
];
}

function getFixes( fixer, context, callNode ) {
const storeName = callNode.arguments[ 0 ].value;
const storeDefinitions = {
core: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to do an optimistic assumption that:
core -> @wordpress/core-data
core/(*) -> @wordpress/${1}
variable would be camelCase( '${1}Store' )

This way we won't need to keep the list up to date

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! How I missed this 😄 5e02dff takes care of it

import: '@wordpress/core-data',
variable: 'coreStore',
},
};
let storeDefinition = storeDefinitions[ storeName ];
if ( ! storeDefinition && storeName.startsWith( 'core/' ) ) {
const storeNameWithoutCore = storeName.substring( 5 );
storeDefinition = {
import: `@wordpress/${ storeNameWithoutCore }`,
variable: storeNameToVariableNames( storeNameWithoutCore ),
};
}
if ( ! storeDefinition ) {
return null;
}
const { variable: variableName, import: importName } = storeDefinition;

const fixes = [
fixer.replaceText( callNode.arguments[ 0 ], variableName ),
];

const imports = context
.getAncestors()[ 0 ]
.body.filter( ( n ) => n.type === 'ImportDeclaration' );
david-szabo97 marked this conversation as resolved.
Show resolved Hide resolved
const packageImports = imports.filter(
( n ) => n.source.value === importName
);
const packageImport =
packageImports.length > 0 ? packageImports[ 0 ] : null;
if ( packageImport ) {
const alreadyHasStore = packageImport.specifiers.some(
( specifier ) => specifier.imported.name === 'store'
);
if ( ! alreadyHasStore ) {
const lastSpecifier = arrayLast( packageImport.specifiers );
fixes.push(
fixer.insertTextAfter(
lastSpecifier,
`,store as ${ variableName }`
)
);
}
} else {
const wpImports = imports.filter( ( n ) =>
david-szabo97 marked this conversation as resolved.
Show resolved Hide resolved
n.source.value.startsWith( '@wordpress/' )
);
const lastImport =
wpImports.length > 0
? arrayLast( wpImports )
: arrayLast( imports );

fixes.push(
fixer.insertTextAfter(
lastImport,
`\nimport { store as ${ variableName } } from '${ importName }';`
)
);
}

return fixes;
}

module.exports = {
meta: {
type: 'problem',
Expand Down Expand Up @@ -131,6 +237,7 @@ module.exports = {
node: callNode.parent,
messageId: 'doNotUseStringLiteral',
data: { argument: callNode.arguments[ 0 ].value },
suggest: getSuggest( context, callNode ),
} );
} );
},
Expand Down