-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix: parse later ES files in eslint --init
(fixes #10003)
#10378
Conversation
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.
LGTM, thanks for contributing!
I'm going to leave this open a little longer so more people can review. Thanks!
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.
Thanks for the PR! I left a few comments.
lib/config/config-initializer.js
Outdated
{ | ||
type: "confirm", | ||
name: "es6Globals", | ||
message: "Are you using ES6 globals (such as `Map` and `Set`)?", |
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.
Nitpick: Can we call this "ES2015 globals", since it's referred to as "ES2015" in the previous question?
lib/config/config-initializer.js
Outdated
{ | ||
type: "confirm", | ||
name: "es6Globals", | ||
message: "Are you using ES6 globals (such as `Map` and `Set`)?", |
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.
Can we reword this as "Do you want to enable..." rather than "Are you using..."? It seems like someone might answer "no" because they're currently not using Map
and Set
, even though they might want to do so in the future.
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'll will update it. BTW, this sentence is copied, not written by me.
lib/config/config-initializer.js
Outdated
message: "Are you using ES6 globals (such as `Map` and `Set`)?", | ||
default: false, | ||
when(answers) { | ||
return answers.ecmaVersion >= 2015; |
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.
What's the reasoning for only asking this question when answers.ecmaVersion >= 2015
? If we're giving users the option to enable ES2015 parsing without ES2015 globals, it seems equally plausible that they might want to enable ES2015 globals without ES2015 parsing.
To me, it seems better to either guess the globals option based on the ecmaVersion
answer (keeping things simpler for the user), or to always ask both questions. I would slightly prefer the former, since the --init
helper is just supposed to address common cases and doesn't need to provide the same level of flexibility as creating a config file manually.
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.
So if the user choose "greater or equals to ES2015", skip this question and enable ES2015 globals?
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, that would be my recommendation.
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.
Should I remove this question? I think, if the user choose ES3 or ES5, it shouldn't enable ES2015 globals, and if the user choose "greater or equals to ES2015", enable it by default.
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.
Agreed, I think it would be best to remove the question and do what you just described.
@not-an-aardvark Updated. |
@not-an-aardvark Are your concerns addressed in this PR? |
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.
LGTM, thanks!
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix
close #10003
What changes did you make? (Give an overview)
When running
eslint --init
, the CLI will ask user the ECMAScript version and whether using es6 globals.Note that I haven't resolved ES7+ globals. This should be in another PR.