-
-
Notifications
You must be signed in to change notification settings - Fork 20
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: add warning message for ESLint v9 with npm --force
flag
#132
Conversation
Hi @aladdin-add!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
--force
flag--force
flag
lib/config-generator.js
Outdated
if (packageManager === "npm" && this.result.installFlags.includes("--force")) { | ||
const pkg = JSON.parse(await path.readFile(this.packageJsonPath, "utf8")); | ||
|
||
log.warn("Note some plugins currently do not support ESLint v9 yet.\nYou will need to use '--force' when installing, or add the following to your package.json:\n"); |
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.
Shouldn't this come in the else
block below? We'd want to log this only when users need to install the dependencies themselves.
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 believe this was intentional. The problem is not when @eslint/create-config
installs dependencies, but when the user runs npm install
anytime after that.
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.
Then it should print warning in both the cases right?
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.
Yeah, that would definitely make sense, but the problem is that in the other case we don't know if it's needed because we don't know which package manager is used in the project. Perhaps we could add something like (not needed if you are using yarn
).
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.
well, I adjusted the message:
when choosing npm:
Note some plugins currently do not support ESLint v9 yet.
You may need to use '--force' when installing, or add the following to your package.json:
"overrides": { "eslint": "^9.5.0" }
others:
Note some plugins currently do not support ESLint v9 yet.
You may need to use '--force' when installing
I'm not a native English speaker, so suggestions for wording improvements are welcome! 😄
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.
we don't know which package manager is used in the project.
We can identify which package manager is being used by checking for lock files?
Note some plugins currently do not support ESLint v9 yet.
this can be changed to Note that some plugins currently do not support ESLint v9 yet.
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.
We can identify which package manager is being used by checking for lock files?
maybe, I'm just not sure it's worthy the efforts - the plugins will add official eslint v9 supports later. :)
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.
We can identify which package manager is being used by checking for lock files?
maybe, I'm just not sure it's worthy the efforts - the plugins will add official eslint v9 supports later. :)
I also think it's not worth the effort at this point, both plugins will probably add official eslint v9 support soon.
f6849a2
to
af55c20
Compare
… flag fixes #125 Signed-off-by: 唯然 <[email protected]>
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! Leaving open for others to review.
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
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!
fixes #125