Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): no unused starting with underscore; small hack for React and removing suggested fix #3124

Merged
merged 2 commits into from Aug 29, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Aug 29, 2022

Summary

This improves noUnusedVariables in three ways and removes the suggestion of removing the unused variables:

1 - It does not flag variables starting with "_";
2 - It does not flag React because some old codebases import React and use it only after JSX transpiling;
3 - It ignores function and class expressions (we are going to deal with these in another rule);

The suggestion was removed, because it suggested removing the first parameter of a function, and of course, completely changing the meaning of the other parameters. We need to think a little bit about what we want to suggest.

Test Plan

> cargo test -p rome_js_analuze -- no_unused

@xunilrj xunilrj requested a review from leops as a code owner August 29, 2022 08:11
@xunilrj xunilrj temporarily deployed to aws August 29, 2022 08:11 Inactive
@cloudflare-pages
Copy link

cloudflare-pages bot commented Aug 29, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5d937e1
Status: ✅  Deploy successful!
Preview URL: https://2fe60ead.tools-8rn.pages.dev
Branch Preview URL: https://feature-underscore-react-no-.tools-8rn.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Aug 29, 2022

@ematipico
Copy link
Contributor

@xunilrj once this PR is merged, could you please add a comment to #2991, and remind to update the documentation of the rule with these two gotchas? - the exemption of React, and variables that start with underscore.

@ematipico ematipico added this to the 0.9.0 milestone Aug 29, 2022
@ematipico ematipico added the A-Linter Area: linter label Aug 29, 2022
@Boshen
Copy link
Contributor

Boshen commented Aug 29, 2022

FYI for parameters, eslint has the after-used option, which I think is pretty nice for a default.

after-used - unused positional arguments that occur before the last used argument will not be checked, but all named arguments and all positional arguments after the last used argument will be checked.

https://eslint.org/docs/latest/rules/no-unused-vars#args

@@ -198,43 +211,4 @@ impl Rule for NoUnusedVariables {

Some(diag)
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could still keep the code action for variables, but instead of removing the whole declaration it could suggest to prefix the name with an underscore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove it for now. And we can return it after it is a little bit more mature.


// Old code import React but do not used directly
// only indirectly after transpiling JSX.
if name.starts_with('_') || name == "React" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for React should only apply to JSX files, although at the moment I don't think we have access to the SourceType for the file in the RuleContext

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add more, we were discussing about implementing JSX parsing to JS files. If that happens in the future, we would need access to that information too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don´t.
It is a little bit hacky now. I expect this to be turned into an option in the future.

@xunilrj
Copy link
Contributor Author

xunilrj commented Aug 29, 2022

FYI for parameters, eslint has the after-used option, which I think is pretty nice for a default.

after-used - unused positional arguments that occur before the last used argument will not be checked, but all named arguments and all positional arguments after the last used argument will be checked.

https://eslint.org/docs/latest/rules/no-unused-vars#args

hum... Honestly not sure about this. One of the reasons I like noUnusedVariables is to get typos. So, for example, the code below would not flag req as unused.

Now... today we only have two ways to suppress these:

1 - Suppression lines;
2 - Starting with an underscore.

Are these fine and enough?

const server = http.createServer(function(req, res) {
    console.log(re); //try to use req but typo
    console.log(res); // correctly uses res
});

@xunilrj xunilrj requested a review from a team August 29, 2022 09:40
@xunilrj xunilrj temporarily deployed to aws August 29, 2022 09:40 Inactive
@xunilrj xunilrj merged commit a6b24ee into main Aug 29, 2022
@xunilrj xunilrj deleted the feature/underscore-react-no-unused-vars branch August 29, 2022 11:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants