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 2 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 @@ -57,6 +57,85 @@ 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
code: `import { select } from '@wordpress/data'; select( 'core' );`,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output: `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.
code: `import { select } from '@wordpress/data'; import { something } from '@wordpress/core-data'; select( 'core' );`,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output: `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.
code: `import { select } from '@wordpress/data'; import { something, } from '@wordpress/core-data'; select( 'core' );`,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output: `import { select } from '@wordpress/data'; import { something,store as coreStore, } from '@wordpress/core-data'; select( coreStore );`,
Copy link
Member

Choose a reason for hiding this comment

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

The output isn't following the coding styles but I guess it's easy to fix with Prettier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather rely on multipass fixes 😄 Basically I apply the fix and save it immediately, thus it fixes all the auto-fixable problems. If you think it's worth fixing this I'm happy to do it though. Let me know

Source: https://eslint.org/docs/developer-guide/working-with-rules

Note: Suggestions will be applied as a stand-alone change, without triggering multipass fixes. Each suggestion should focus on a singular change in the code and should not try to conform to user defined styles. For example, if a suggestion is adding a new statement into the codebase, it should not try to match correct indentation, or confirm to user preferences on presence/absence of semicolons. All of those things can be corrected by multipass autofix when the user triggers it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it makes perfect sense. I considered it when mentioning Prettier, but now I see this recommendation and I totally get the idea. Even when you add spaces here, something else might add another variable to the import or reorder them and finally Prettier will do its own changes regardless.

},
],
},
],
},
{
// Replace core with coreStore. Store import already exists. It shouldn't modify the import, just replace the literal with the store definition.
code: `import { select } from '@wordpress/data'; import { store as coreStore } from '@wordpress/core-data'; select( 'core' );`,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output: `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.
code: `import { a } from './a'; import { select } from '@wordpress/data'; import { b } from './b'; select( 'core' );`,
errors: [
{
suggestions: [
{
desc:
'Replace literal with store definition. Import store if neccessary.',
output: `import { a } from './a'; import { select } from '@wordpress/data';\nimport { store as coreStore } from '@wordpress/core-data'; import { b } from './b'; select( coreStore );`,
},
],
},
],
},
];
const errors = [
{
Expand All @@ -66,5 +145,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
),
} );
122 changes: 122 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,7 @@
function arrayLast( array ) {
return array[ array.length - 1 ];
}

function getReferences( context, specifiers ) {
const variables = specifiers.reduce(
( acc, specifier ) =>
Expand Down Expand Up @@ -85,6 +89,123 @@ 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',
},
'core/block-editor': {
import: '@wordpress/block-editor',
variable: 'blockEditorStore',
},
'core/block-directory': {
import: '@wordpress/block-directory',
variable: 'blockDirectoryStore',
},
'core/blocks': {
import: '@wordpress/blocks',
variable: 'blocksStore',
},
'core/editor': {
import: '@wordpress/editor',
variable: 'editorStore',
},
'core/notices': {
import: '@wordpress/notices',
variable: 'noticesStore',
},
'core/reusable-blocks': {
import: '@wordpress/reusable-blocks',
variable: 'reusableBlocksStore',
},
'core/keyboard-shortcuts': {
import: '@wordpress/keyboard-shortcuts',
variable: 'keyboardShortcutsStore',
},
'core/edit-post': {
import: '@wordpress/edit-post',
variable: 'editPostStore',
},
'core/edit-site': {
import: '@wordpress/edit-site',
variable: 'editSiteStore',
},
'core/interface': {
import: '@wordpress/interface',
variable: 'interfaceStore',
},
'core/viewport': {
import: '@wordpress/viewport',
variable: 'viewportStore',
},
'core/rich-text': {
import: '@wordpress/rich-text',
variable: 'richTextStore',
},
};
if ( ! storeDefinitions[ storeName ] ) {
return null;
}
const { variable: variableName, import: importName } = storeDefinitions[
storeName
];

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 i = packageImports.length > 0 ? packageImports[ 0 ] : null;
if ( i ) {
const alreadyHasStore = i.specifiers.some(
( s ) => s.imported.name === 'store'
);
if ( ! alreadyHasStore ) {
const lastSpecifier = arrayLast( i.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 +252,7 @@ module.exports = {
node: callNode.parent,
messageId: 'doNotUseStringLiteral',
data: { argument: callNode.arguments[ 0 ].value },
suggest: getSuggest( context, callNode ),
} );
} );
},
Expand Down