-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[New] Align no-deprecated
rule with React 18 deprecations
#3548
[New] Align no-deprecated
rule with React 18 deprecations
#3548
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3548 +/- ##
==========================================
+ Coverage 97.60% 97.62% +0.01%
==========================================
Files 132 132
Lines 9285 9286 +1
Branches 3395 3392 -3
==========================================
+ Hits 9063 9065 +2
+ Misses 222 221 -1
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
'ReactDOM.render(element, container);', | ||
'ReactDOM.unmountComponentAtNode(container);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to keep these test cases with the react version set to 17.999.999
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one more test to cover this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, keep the "valid" tests cases, because these remain valid in react < 18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, keep the "valid" cases, because these remain valid in react < 18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot leave it where it was before, React version isn't specified there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right - i mean, edit the existing rule to add the react version, so that "a passing test for ReactDOM.render" remains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React version can be specified using eslint settings.react.version
property which is demonstrated in the added tests. Introducing multiple sources to determine React version doesn't seem like a good idea to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb Is there anything else that prevents the PR from being merged? I believe the requested scenario has been covered:
eslint-plugin-react/tests/lib/rules/no-deprecated.js
Lines 122 to 132 in 1375bd1
// React < 18 | |
{ | |
code: ` | |
import { render, hydrate } from 'react-dom'; | |
import { renderToNodeStream } from 'react-dom/server'; | |
ReactDOM.render(element, container); | |
ReactDOM.unmountComponentAtNode(container); | |
ReactDOMServer.renderToNodeStream(element); | |
`, | |
settings: { react: { version: '17.999.999' } }, | |
}, |
5e648c8
to
838ac07
Compare
The list of deprecated APIs in React 18.0.0:
ReactDOM.render
ReactDOM.hydrate
ReactDOM.unmountComponentAtNode
ReactDOMServer.renderToNodeStream
Ref: https://github.com/facebook/react/blob/main/CHANGELOG.md#deprecations
Migration guide: https://beta.reactjs.org/blog/2022/03/08/react-18-upgrade-guide