feat(rome_js_analyze): lint rule to check if react hooks are at top level #3927

xunilrj · 2022-12-03T16:08:59Z

Summary

Implements https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

Codegen and just

This PR also improve codegen around lint rules:

> just
just --list -u
Available recipes:
    default
    codegen
    documentation
    new-lintrule path name
    test-lintrule name
    check-ready

just new-lintrule will create everything needed to start a rule from the scratch. The template can sure be improved, but as a first version I think it is fine.

just test-lintrule will run all the rule tests.

just check-ready check if the branch is ready to be merged.

Test Plan

> cargo test -p rome_js_analyze -- use_hook_at_top_level

netlify · 2022-12-03T16:09:12Z

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	`64c6b34`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/639229cab770330008ff0de4
😎 Deploy Preview	https://deploy-preview-3927--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-12-03T16:17:35Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44936	44936	0
Failed	943	943	0
Panics	0	0	0
Coverage	97.94%	97.94%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	1684	1684	0
Failed	4262	4262	0
Panics	0	0	0
Coverage	28.32%	28.32%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12397	12397	0
Failed	3860	3860	0
Panics	0	0	0
Coverage	76.26%	76.26%	0.00%

calibre-analytics · 2022-12-03T17:39:51Z

Comparing feat(rome_js_analyze): lint rule to check if react hooks are at top level Snapshot #6 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	395ms ^from 307ms	0.044 ^from 0.009	0ms ^no change
Chrome Desktop ^{Chrome Desktop • Cable}	395ms ^from 307ms	0.044 ^from 0.005	0ms ^no change
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	235ms ^from 259ms	0.078 ^from 0.009	0ms ^no change
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	1.14s ^from 1.07s	0.024 ^from 0.009	0ms ^no change

1 page tested

Home

Browser previews

Chrome Desktop	iPhone, 4G LTE	Motorola Moto G Power, 3G connection

Most significant changes

	Value	Budget
Total JavaScript Size in Bytes Chrome Desktop	1.33 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes iPhone, 4G LTE	1.33 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	1.33 MB ^{from 86.8 KB}
Cumulative Layout Shift Chrome Desktop	0.044 ^from 0.005
Cumulative Layout Shift iPhone, 4G LTE	0.078 ^from 0.009

_{15 other significant changes: Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection}

_{Calibre: Site dashboard | View this PR | Edit settings | View documentation}

leops · 2022-12-05T14:16:25Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_hook_at_top_level.rs

+
+        let call = ctx.query();
+        let hook_name_range = call.callee().ok()?.syntax().text_trimmed_range();
+        if react_hook_configuration(call, &options.hooks_config).is_some() {


While it makes sense to only inspect hooks that are present in the configuration for useExhaustiveDependencies (since we need to know if it's a hook with a dependencies array), for useHookAtTopLevel we could detect all function calls where the identifier of the callee starts with a lowercase "use" (if necessary we could narrow it down to only inspect "components functions", with a name starting with an uppercase letter)

Additionally since we're inspecting the call chain, we could enforce the "use*" naming convention on all intermediate functions between hooks and components. For instance here:

function Component() { intermediate(); } function intermediate() { useEffect(); }

We could show a diagnostic on intermediate suggesting to rename it to useIntermediate (but that kind of analysis doesn't really suit the useHookAtTopLevel name)

While it makes sense to only inspect hooks that are present in the configuration for useExhaustiveDependencies (since we need to know if it's a hook with a dependencies array), for useHookAtTopLevel we could detect all function calls where the identifier of the callee starts with a lowercase "use" (if necessary we could narrow it down to only inspect "components functions", with a name starting with an uppercase letter)

Yeah. I have some plans to improve this.
Currently all react rules will need to query for JsCallExpression and check if is a React call or not. Probably with 99% false rate. My plan is to create a service, or inside the semantic model or not, to flag react calls.

Then you would create something like

let calls = ctx.query().all_react_class();

or even

let hooks = ctx.query().all_hooks();

But for now, since we don't have this yet, I would keep this as it is.

Additionally since we're inspecting the call chain, we could enforce the "use*" naming convention on all intermediate functions between hooks and components. For instance here:

We could show a diagnostic on intermediate suggesting to rename it to useIntermediate (but that kind of analysis doesn't really suit the useHookAtTopLevel name)

This is actually one of the new hooks rules we will create for vNext.

ematipico

I left some feedback.

Overall I think the PR is amazing, I think the diagnostics are even better than the ones of the eslint plugin! 🚀

I don't have huge concerns, although:

we should review the wording of our diagnostics and try to make them beginner-friendly, the use of "Top Level" is not very known to beginners or even mid-level react developers;
about just, we should update the contribution guidelines with the new commands and how just should be installed on the different platforms (even a link is sufficient). For example, I wouldn't know how to install and use it without some guidelines;

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_hook_at_top_level.rs

crates/rome_js_semantic/src/semantic_model/model.rs

crates/rome_js_semantic/src/semantic_model/reference.rs

ematipico · 2022-12-07T17:09:04Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_hook_at_top_level.rs

+                    for call in model.all_calls(&f) {
+                        let node = call.tree();
+                        let path = path.clone();
+                        calls.push(CallPath { call: node, path });
+                    }


Not sure if it makes a difference:

calls.extend( model.all_calls(&f).into_inter().map(|call| { let node = call.tree(); let path = path.clone(); CallPath { call: node, path } }) )

It is the same thing. But I tend to consider expressions with closures more complex, including for the compiler.

ematipico · 2022-12-07T17:12:34Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_hook_at_top_level.rs

+                        rule_category!(),
+                        hook_name_range,
+                        markup! {
+                            "This hook is not necessarily being called from the Top Level of the component function."


I wonder if we can align our wording with the one from the eslint plugin:

React Hook "useEffect" is called conditionally. React Hooks must be called in the exact same order in every component render.

This would help the transition to our rules. I expect users to be accustomed to the wording provided by the plugin. What concerns me is the use of "Top level" which, I think. it's not a beginner-friendly concept.

Done. What do you think now?

Way better now! Thank you for looking into this!

ematipico

Looks good to me! Could you please update the contribution guidelines or create an issue to track it? There's some places where we should update:

https://github.com/rome/tools/blob/main/CONTRIBUTING.md#generated-files
https://github.com/rome/tools/blob/main/crates/rome_analyze/CONTRIBUTING.md#code-generation
https://github.com/rome/tools/blob/main/crates/rome_analyze/CONTRIBUTING.md#create-your-first-rule here maybe we could add your new just command

xunilrj changed the title ~~lint rule to check if react hooks are at top level~~ feat(rome_js_analyze): lint rule to check if react hooks are at top level Dec 3, 2022

xunilrj marked this pull request as ready for review December 3, 2022 16:58

xunilrj requested review from leops, ematipico and a team as code owners December 3, 2022 16:58

leops reviewed Dec 5, 2022

View reviewed changes

xunilrj force-pushed the feature/use-hook-at-top-level branch from c60444a to 9506ace Compare December 5, 2022 22:50

ematipico reviewed Dec 7, 2022

View reviewed changes

xunilrj force-pushed the feature/use-hook-at-top-level branch from 3ef943c to 4504d38 Compare December 7, 2022 23:07

leops approved these changes Dec 8, 2022

View reviewed changes

ematipico approved these changes Dec 8, 2022

View reviewed changes

ematipico added the A-Linter Area: linter label Dec 8, 2022

ematipico added this to the Next milestone Dec 8, 2022

xunilrj added 13 commits December 8, 2022 18:11

lint rule to check if react hooks are at top level

93f1ac8

support for initializers

a21db95

improv new lint rule codegen

3f1c1c7

improv justfile

485152b

improv justfile

c32dd40

improv justfile

d885b5c

touch as private

a7e0dee

rebasing

42715de

fmt and clippy issues

1a54daf

pr adjustments

a14062f

rebasing

3c62f0d

just explanation on contributing files

08fcc6c

just explanation on contributing files

64c6b34

xunilrj force-pushed the feature/use-hook-at-top-level branch from 4504d38 to 64c6b34 Compare December 8, 2022 18:15

xunilrj merged commit cf98b46 into main Dec 8, 2022

xunilrj deleted the feature/use-hook-at-top-level branch December 8, 2022 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): lint rule to check if react hooks are at top level #3927

feat(rome_js_analyze): lint rule to check if react hooks are at top level #3927

xunilrj commented Dec 3, 2022 •

edited

Loading

netlify bot commented Dec 3, 2022 •

edited

Loading

github-actions bot commented Dec 3, 2022 •

edited

Loading

calibre-analytics bot commented Dec 3, 2022 •

edited

Loading

Browser previews

Most significant changes

leops Dec 5, 2022

xunilrj Dec 5, 2022 •

edited

Loading

xunilrj Dec 5, 2022 •

edited

Loading

ematipico left a comment

ematipico Dec 7, 2022

xunilrj Dec 7, 2022

ematipico Dec 7, 2022

xunilrj Dec 7, 2022

ematipico Dec 8, 2022

ematipico left a comment

feat(rome_js_analyze): lint rule to check if react hooks are at top level #3927

feat(rome_js_analyze): lint rule to check if react hooks are at top level #3927

Conversation

xunilrj commented Dec 3, 2022 • edited Loading

Summary

Codegen and just

Test Plan

netlify bot commented Dec 3, 2022 • edited Loading

✅ Deploy Preview for docs-rometools ready!

github-actions bot commented Dec 3, 2022 • edited Loading

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

calibre-analytics bot commented Dec 3, 2022 • edited Loading

1 page tested

Browser previews

Most significant changes

leops Dec 5, 2022

Choose a reason for hiding this comment

xunilrj Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

xunilrj Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

ematipico Dec 7, 2022

Choose a reason for hiding this comment

xunilrj Dec 7, 2022

Choose a reason for hiding this comment

ematipico Dec 7, 2022

Choose a reason for hiding this comment

xunilrj Dec 7, 2022

Choose a reason for hiding this comment

ematipico Dec 8, 2022

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

xunilrj commented Dec 3, 2022 •

edited

Loading

netlify bot commented Dec 3, 2022 •

edited

Loading

github-actions bot commented Dec 3, 2022 •

edited

Loading

calibre-analytics bot commented Dec 3, 2022 •

edited

Loading

xunilrj Dec 5, 2022 •

edited

Loading

xunilrj Dec 5, 2022 •

edited

Loading