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

Testing: Add ESLint restricted syntax for truthy length rendering #14579

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 22, 2019

This pull request seeks to add a new ESLint restricted syntax to prevent the use of a length property in a boolean expression for rendering.

The problem is that if length yields a value of 0, it will be rendered verbatim, which is not intended. The presumed intended behavior is to render the right-hand expression only if length is non-zero, which should be expressed as either length > 0 or as explicitly coercing to a boolean value !! length.

See CodePen example: https://codepen.io/aduth/pen/dWwzrL

See ASTExplorer demo: https://astexplorer.net/#/gist/9ec7cf3fb53e63775defbc0e532b4cc8/487ccade54ed4c02fc846d1211083a9eac79f23f

Testing instructions:

I'm not positive how to trigger the zero length here. I discovered as an otherwise buggy state of an in-progress branch which caused multiple toolbars to be rendered (screenshot).

Repeat testing instructions from #14233

Verify that if the changes to FormatToolbar are undone in your local copy of the branch, proceeding to run npm run lint should produce an error.

@aduth aduth added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Mar 22, 2019
@aduth aduth requested a review from ellatrix March 22, 2019 14:53
@@ -92,6 +92,10 @@ module.exports = {
selector: 'CallExpression[callee.name="withDispatch"] > :function > BlockStatement > :not(VariableDeclaration,ReturnStatement)',
message: 'withDispatch must return an object with consistent keys. Avoid performing logic in `mapDispatchToProps`.',
},
{
Copy link
Member

Choose a reason for hiding this comment

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

This rule seems generic enough that we may consider add it in packages/eslint-plugin/configs/custom.js

Copy link
Member Author

@aduth aduth Mar 22, 2019

Choose a reason for hiding this comment

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

Yeah, I'd thought about it. And even as a proper rule, it wouldn't be too difficult to implement a fixer for it. I also don't love no-restricted-syntax for the sole reason that when someone chooses to disable it, it's very unclear to the next person what exactly is being disabled (no-restricted-syntax is a lot less meaningful than something like wordpress/no-truthy-length-rendering). Also also, it could be a reasonable suggestion to offer upstream to eslint-plugin-react.

That all being said, I saw it as something of a process of being "promoted" from a very immediately actionable solution, to something which has proved its value as generally useful to be included as part of a custom rule or a default configuration. It took me all of a few minutes to put together this pull request and have an immediate benefit, vs. the time cost in forming into a new rule. I also feel some regret by my own over-eagerness to include rules in default configurations, such as @wordpress/dependency-group, @wordpress/valid-sprintf, and frankly those exact no-restricted-syntax in custom.js you refer to (__ is not something which should be restricted except by opt-in with choosing to use @wordpress/i18n).

@aduth aduth force-pushed the add/eslint-no-boolean-logic-length-render branch from a126b36 to dd911a6 Compare March 25, 2019 20:05
@aduth
Copy link
Member Author

aduth commented Mar 25, 2019

The component was updated in #14497, so I've rebased the pull request to contain only the ESLint rule addition.

.eslintrc.js Show resolved Hide resolved
@aduth aduth merged commit 1f0fea9 into master Mar 26, 2019
@aduth aduth deleted the add/eslint-no-boolean-logic-length-render branch March 26, 2019 19:32
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants