-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
linting: disallow t.identifier("undefined") in plugins #6096
Conversation
8410210
to
c0326eb
Compare
…") with buildUndefinedNode(), update tests
c0326eb
to
06c1361
Compare
Nice. We could create a separate project from this and publish it ( |
|
I don't really want us to maintain another project lol, if we wanted it should go in existing package or we just use it internally until it makes more sense to use elsewhere. And regarding the robustness, may need to check eslint's api otherwise it's easy to do the same thing in a babel transform |
@hzoo to clarify, the proposed babel transform would replace instances of |
it would either be a codemod or it would just throw like a linting rule (this is just if you aren't able to implement the same functionality with eslint, otherwise can just use what you have which covered a bunch of cases we have in the codebase already) |
@hzoo ok, cool. i think it should be doable with eslint. gonna spend a bit more time in it, will update here when i do |
Doesn't have to be in this pr either because it already provides a benefit |
okay! well, if this gets deemed ready-to-merge i'll submit improvements in a followup PR |
) { | ||
context.report( | ||
node, | ||
"prefer path.scope.buildUndefinedNode() to creating an undefined identifier directly" |
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.
to creating -> to create (please correct me if i'm wrong)
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.
hmm, i think the wording i chose here might be strange
perhaps "to" -> "instead of" ?
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.
I would say:
t.identifier("undefined") is prohibited, please use
path.scope.buildUndefinedNode()
instead.
It's not really prohibited but I think it's clear enought
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.
👍 I agree with that
also, maybe "restricted" instead of "prohibited"? that seems to be what the core eslint rules use in phrases like this
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.
yes, it sounds great
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.
Maybe just:
Use
path.scope.buildUndefinedNode()
to create an undefined identifier directly.
Question: is this rule autofixable? |
@nicolo-ribaudo unfortunately, i think it's not always easily autofixable. for example, in https://github.com/babel/babel/pull/6076/files#diff-c560784a38cbcda22571f3ccc9511964R32 |
#5971 should fix this. |
gonna close this, assuming that #5971 fixes this as mentioned above feel free to reopen if there's any value in adding this rule anyway |
@kedromelon #5971 shouldn't fix the t.identfier('undefined') issue though? otherwise we would have to update the places where your linting rule already caught issues? If anything should at least change those files if not add the rule anyway |
oh, okay. perhaps i misunderstood that PR -- how does #5971 relate here then? i'm not sure i follow |
I was incorrect, we don't forbid it in #5971. I thought we had a |
@jridgewell gotcha, thanks for clarifying! |
wasn't able to figure out improvements to robustness -- skimmed the eslint docs and also checked out some common plugins with similar rules (mocha and lodash had a few that seemed applicable), seems like this is the most we're able to get out of eslint (if anyone has some insight to drop though on how we could accomplish this, please let me know) next i'll look into doing it via a babel transform instead, if that still seems like a good alternative |
@hzoo mentioned the idea to lint to disallow
t.identifier("undefined")
in #6076 (comment)this PR implements a basic eslint rule to enforce this, as well as changes the remaining instances to use
buildUndefinedNode()
questions:
t.identifier
and the function is called via that variable it will not be caught"undefined"
and passed tot.identifier
, this will not be caught (e:const foo = "undefined"; t.identifier(foo)
)eslint_rules/
directory at rootpackages/babel-plugin-*
-- should this be applied more broadly?thanks for reading! hope this is helpful 😄