update eslint config to use v6 and updated react rule options [PLAT-22391] #35
Conversation
|
What is the process for version number increment and NPM release? I would like to integrate these changes once landed into a PLAT PR for the referenced Jira ticket. |
jacobroufa
force-pushed
the
update-eslint
branch
from
717c8fc
to
11937a3
Compare
jacobroufa
force-pushed
the
update-eslint
branch
from
11937a3
to
2875f33
Compare
| @@ -40,7 +48,7 @@ module.exports = { | |||
| 'react/jsx-max-props-per-line': [0, { 'maximum': 1 }], | |||
| // Prevent usage of .bind() and arrow functions in JSX props | |||
| // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md | |||
| 'react/jsx-no-bind': 2, | |||
| 'react/jsx-no-bind': [2, { 'allowArrowFunctions': true }], | |||
There was a problem hiding this comment.
This rule is really outdated, we should consider disabling it in a future iteration. It's also not applicable to hooks-based components since they create functions every render. It's fine as-is for now.
https://reactjs.org/docs/faq-functions.html#is-it-ok-to-use-arrow-functions-in-render-methods
There was a problem hiding this comment.
Per our conversation this morning, this is a style preference, and where it would error prior if there was a this.memberFunction.bind(this) or () => doSomething() in a component callback property, the addition of allowArrowFunctions ensures that we can do the latter and still retain the safety of the reminder to bind a class method.
| // Prevent variables used in JSX to be incorrectly marked as unused | ||
| // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-uses-vars.md | ||
| 'react/jsx-uses-vars': 2, | ||
| // Prevent missing parentheses around multilines JSX | ||
| // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/wrap-multilines.md | ||
| 'react/jsx-wrap-multilines': [2, { |
There was a problem hiding this comment.
It looks like this may conflict with Prettier, can we disable it in favor of using whatever Prettier outputs? prettier/prettier#1009 (comment)
There was a problem hiding this comment.
I don't think this conflicts -- the issue is more than two years old and was inconclusive. In my current experience there is no problem. If we consider adding prettier to this javascript project, there might be a conversation we can have at that time about where to place an appropriate rule.
| // Prevent usage of setState in componentDidMount | ||
| // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-did-mount-set-state.md | ||
| 'react/no-did-mount-set-state': [2, 'allow-in-func'], | ||
| 'react/no-did-mount-set-state': 2, |
There was a problem hiding this comment.
I may be missing some context, this seems like a change that is more restrictive? It previously allowed callback functions... jsx-eslint/eslint-plugin-react@4c167eb#diff-7363406d1b2e67feb3b50cbde0fff1caR60
Edit: Nope, I linked to outdated documentation. The rule is equivalent now to the way it was previously.
There was a problem hiding this comment.
The rule was changed to the inverse -- disallow-in-func -- so the removal of that configuration property is equivalent with the new version of eslint.
There was a problem hiding this comment.
@billpull let me know please if there's any more I need to do for incrementing the version and creating a new NPM release.
|
@jacobroufa looks good to me. What are your thoughts on whether this should be a |
|
@billpull thanks, I don't have a preference. As long as the other projects are pinned to specific versions and I can likewise do the same for Dashboard moving forward, I think that either will do. |
|
@jacobroufa opted for v0.2.0 since seems like a decent gap between eslint versions and the structure of properties. #36 also updated |
When updating our use of ESLint for the Dashboard there were several react rules that required an updated format -- this PR sets us up to use a much newer version of ESLint and updates those rule formats accordingly.