-
-
Notifications
You must be signed in to change notification settings - Fork 475
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
feat(rule/solidjs): add no-react-specific-props #2427
feat(rule/solidjs): add no-react-specific-props #2427
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you @marvin-j97 for taking some initiative! I will take a look at the rules in the coming days. As for now, all rules must pass an incubation phase, hence the nursery group. During this incubation phase, we can implement the rule however we want. Then, we will decide the final group :) |
CodSpeed Performance ReportMerging #2427 will improve performances by 6.06%Comparing Summary
Benchmarks breakdown
|
@ematipico Snapshot tests are working now, the JSX fixtures were not delimited by semicolon... |
Awesome @marvin-j97! Could you open an umbrella issue to track all the rules we need to implement, with relative documentation, please? That would help to have a central place where we can coordinate the work and maybe seek more volunteers. |
makes no difference
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noReactSpecificProps/invalid.jsx.snap
Outdated
Show resolved
Hide resolved
d09ed29
to
58d98c8
Compare
…om/marvin-j97/biome into rules/solidjs/noReactSpecificProps
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.
The rule now is easier to understand and to handle. I left few suggestions around coding and docs. We can merge the PR after we addressed them
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
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.
Still a couple of changes, and we're good to go
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_react_specific_props.rs
Outdated
Show resolved
Hide resolved
…ps.rs Co-authored-by: Emanuele Stoppa <[email protected]>
I think the website was moved away before the PR was merged or smth .. leading to a single file empty website folder 🤔 |
You're an eagle!! Good eye |
Summary
SolidJS has a couple of rules that may be useful, especially when transitioning from React, see #3 (comment).
Test Plan
invalid.jsx only works for 2 out of 3 test cases right now. I'm essentially using the same implementation asAlso I'm not sure if the rule should be part ofno-duplicate-jsx-props
, but I don't know why className with a brace expression is not getting picked up yet... I'm not really familiar with Biome's codebase yet, so this PR is an attempt to get into it, this is probably an easy fix.nursery
or if SolidJS rules may need a new group altogether...