This repository was archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_js_analyze): add support for spread props in various rules #3444
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
b79a3d0 to
83cd5ee
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
|
Should this be a rule on itself? "Spread should be at the beginning on JSX bla bla bla". |
I think we had a rule that actually wanted to prevent the use of spread props, but it doesn't play well when using other components (like UI libraries) or library authors. If we had a some type information, we could make it a rule. |
MichaReiser
reviewed
Oct 21, 2022
crates/rome_js_analyze/src/analyzers/nursery/use_key_with_click_events.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_valid_anchor.rs
Outdated
Show resolved
Hide resolved
2bd5398 to
4ef322c
Compare
4ef322c to
8d05644
Compare
leops
reviewed
Oct 24, 2022
| ///``` | ||
| /// | ||
| /// ```jsx, | ||
| /// <div onClick={() => {}} {...spread}></div> |
There was a problem hiding this comment.
I'm not sure I get why onClick {...spread} is valid but {...spread} onClick is not, in both case the spread expression may inject an onKey* event we don't know about ?
There was a problem hiding this comment.
Yeah you're right, good catch
8d05644 to
5a708b8
Compare
leops
approved these changes
Oct 25, 2022
5a708b8 to
cceccad
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds support for checking the presence of spread attributes inside some rules around JSX. The position where the spread attribute is positioned is important. These two snippets yield two different results.
Considering that we don't have any typing information around the spread props, it's safe to assume that when a spread attribute is after the attribute that triggers the rule, the rule should not be triggered anymore, because the spread attribute might contain the correct attribute.
Test Plan
I added new test cases in the rules that have been affected