-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Update: separate eslint --init #79
Conversation
86b7924
to
a8643eb
Compare
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 picking this up. Just a few details to address.
@eslint/eslintrc will not be removed.
58193e4
to
2010343
Compare
2010343
to
15f573d
Compare
Co-authored-by: Nicholas C. Zakas <[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.
I think we have a good idea about the direction of this now. Thanks for finishing it up.
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.
This looks like enough to guide the implementation in the right direction. Thanks!
Moving to final commenting. |
- the following dependencies can be removed in eslint repo: | ||
- enquirer | ||
- semver | ||
- Whenever `--init` command is being used, show a warning(e.g. "You can also run this command directly using 'npm init @eslint/config'") and run `npm init @eslint/config` using `child_process` of the native node modules. |
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 just show the message and let the user run the command manually?
One of the obstacles for supporting yarn in eslint --init
(eslint/eslint#13756) was a scenario where the config initialization reinstalls local eslint, and then yarn intentionally breaks the whole process.
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.
👍 good point! will update 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.
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.
Doesn't that issue only apply when using yarn
? If we run npm init @eslint/config
, aren't we okay?
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.
It will be an issue when users are using some different package managers than npm(yarn/pnpm/...).
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.
that also works. though I think better to recommend yarn users run yarn create @eslint/config
. :)
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.
@eslint/create-config
will require()
local ESLint to format the .eslintrc.js
config file. Since yarn 2 doesn't create node_modules
, I think that @eslint/create-config
when run through npm init
won't be able to require()
any locally installed packages.
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 add to my previous comment, eslint --init
doesn't officially support yarn yet, but yarn eslint --init
currently works fine when provided answers don't require installing dependencies. I'd expect the formatting step to fail when user runs yarn eslint --init
and then we run npm init @eslint/config
as a child process.
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.
Given that we are in final commenting and there’s a lot of “I think” comments that are best answered by implementing the package and testing it, I move that we approve this RFC and hold further speculation until there is a prototype to play with.
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. We'll certainly publish the new package first, and then we can experiment.
Co-authored-by: Milos Djermanovic <[email protected]>
There is consensus on moving forward, so it’s time to merge! |
* Docs: separate eslint --init * chore: review suggestions * chore: review suggestions * chore: review suggestions * chore: review suggestions * Update README.md @eslint/eslintrc will not be removed. * chore: review suggestions * Update designs/2021-init-command-eslint-cli/README.md Co-authored-by: Nicholas C. Zakas <[email protected]> * Update README.md * Update designs/2021-init-command-eslint-cli/README.md Co-authored-by: Milos Djermanovic <[email protected]> * chore: rm unused deps Co-authored-by: Nicholas C. Zakas <[email protected]> Co-authored-by: Milos Djermanovic <[email protected]>
rendered: https://github.com/aladdin-add/rfcs/tree/init-command-eslint-cli/designs/2021-init-command-eslint-cli
Summary
init
command from main repo (eslint) to a new repo named@eslint/create-config
.Related Issues
this is following #58