[RULE] No Destructuring props

N0tExisting commented 2 years ago

As mentioned in the docks here, destructuring props will cause you to lose reactivity, so so making a rule against it would be very useful to catch such unexpected errors

joshwilsonvu commented 2 years ago

Thanks for the suggestion! I have a plan to implement a rule like this, but it may be a part of a larger rule to track proper use of reactivity in general—it's a complex topic so the best approach is still up in the air. A PR is welcome, and I'll prioritize work on this.

N0tExisting commented 2 years ago

Personally I'd check if the first argument of a function that returns jsx is being destructured and only then error.

I dislike the idea of a big rule, as the point of a rule is to check for a single mistake, and having a rule check for multiple mistakes is anoying for multiple reasons.

I'd love to make a PR, However I have no experience writing eslint rules, so I might have to learn a bit.

joshwilsonvu commented 2 years ago

I'm curious if you have experience using the react-hooks/rules-of-hooks or react-hooks/exhaustive-deps rules? If so, do you dislike them, and why? These rules check for multiple mistakes, but these mistakes are related to a single concern, as "reactivity" would be the concern here.

Either way, I may still implement a rule like you described, if only in the short term. I'm sure it will provide value even if it doesn't check reactivity everywhere.

N0tExisting commented 2 years ago

I don't have experience with the rules you mentioned
Personally I'd dislike a rule like the one you have right now, because something like destructured props is a diffrent issue that a computation outside a root. Also it would be a pain to maintain, as you'd have a massive file. I think a better idea would be to have a config that contains all the reactivity rules (which is then added to the default config once the rules are finalized)

joshwilsonvu commented 2 years ago

I appreciate the input, thanks again. I suppose multiple rules could offer better error messages. One of my concerns was potentially duplicating computationally expensive work across multiple rules, but there should be a way to address that problem.

N0tExisting commented 2 years ago

It seems like eslint plugin react has a rule for consistency with destructuring: https://github.com/yannickcr/eslint-plugin-react/blob/HEAD/docs/rules/destructuring-assignment.md

joshwilsonvu commented 2 years ago

Thanks for the tip, I’ve used that one before. You could certainly use that rule for now before my no-destructure rule is finished, but I’m not sure it works in the parameter list. I’m making progress on it, but there’s a lot to do:

Reporting:

[x] Detecting components
[x] Detecting prop destructuring in the parameter list ~~Detecting prop destructuring in the component body~~

Fixing:

[x] Replacing destructured parameter list with props
[x] Replacing usages of destructured props with accesses to props fields
[x] Inserting a call to mergeProps where an AssignmentPattern ({ x = 5 }) is used
[x] Inserting a call to splitProps where rest is used

Hopefully at least the reporting part of it doesn’t take much longer.

joshwilsonvu commented 2 years ago

Got this finished and released with version 0.3.0! I'd love any feedback if you give it a try.

N0tExisting commented 2 years ago

Are you sure the rule is included? It doesn't seem like it!

joshwilsonvu commented 2 years ago

Really? I see it getting loaded up in the NPM package: https://unpkg.com/eslint-plugin-solid@0.3.0/dist/index.js

I'll try to reproduce in a bit.

N0tExisting commented 2 years ago

Really? I see it getting loaded up in the NPM package: https://unpkg.com/eslint-plugin-solid@0.3.0/dist/index.js

I'll try to reproduce in a bit.

Nevermind, I just missed it in the src/index.ts, Sorry for bothering you

joshwilsonvu commented 2 years ago

No problem, better safe than sorry!

solidjs-community / eslint-plugin-solid

[RULE] No Destructuring props #2