Testing: Add ESLint restricted syntax for truthy length rendering#14579
Conversation
| selector: 'CallExpression[callee.name="withDispatch"] > :function > BlockStatement > :not(VariableDeclaration,ReturnStatement)', | ||
| message: 'withDispatch must return an object with consistent keys. Avoid performing logic in `mapDispatchToProps`.', | ||
| }, | ||
| { |
There was a problem hiding this comment.
This rule seems generic enough that we may consider add it in packages/eslint-plugin/configs/custom.js
There was a problem hiding this comment.
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).
a126b36 to
dd911a6
Compare
|
The component was updated in #14497, so I've rebased the pull request to contain only the ESLint rule addition. |
This pull request seeks to add a new ESLint restricted syntax to prevent the use of a
lengthproperty in a boolean expression for rendering.The problem is that if
lengthyields a value of0, it will be rendered verbatim, which is not intended. The presumed intended behavior is to render the right-hand expression only iflengthis non-zero, which should be expressed as eitherlength > 0or 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
FormatToolbarare undone in your local copy of the branch, proceeding to runnpm run lintshould produce an error.