Skip to content
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

Merged
merged 11 commits into from
Jun 10, 2021
23 changes: 10 additions & 13 deletions designs/2021-init-command-eslint-cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@

## Summary

- Move the `init` command from main repo ([eslint](https://github.com/eslint/eslint)) to a new repo named `@eslint/create`.
- Move the `init` command from main repo ([eslint](https://github.com/eslint/eslint)) to a new repo named `@eslint/create-config`.
- Remove the auto-config.

## Motivation

Currently the whole `init` command is being shipped with the cli in the main repo. Though its not that much of size (roughly ~0.6MB for the [eslint/lib/init](https://github.com/eslint/eslint/tree/master/lib/init)), this command is not a type of command that we need everyday or everytime running eslint. It is mainly used when creating a new project or adding eslint to a project for the first time. So if we move this to a separate package that is meant to use in the command line,
We will make use of tools like `npx` to simply run `npx @eslint/create` or using `npm` to run `npm init @eslint` or using `yarn` to run `yarn create @eslint` single time for a project instead of having it in the core project.
We will make use of tools like `npx` to simply run `npm init @eslint/config` or using `npm` to run `npx @eslint/create-config` or using `yarn` to run `yarn create @eslint/config` single time for a project instead of having it in the core project.

## Detailed Design

Expand All @@ -26,14 +26,14 @@ We will make use of tools like `npx` to simply run `npx @eslint/create` or using
used. Be sure to define any new terms in this section.
-->

The implementation is mainly migrating the code from main repo to `@eslint/create`.
The implementation is mainly migrating the code from main repo to `@eslint/create-config`.

The approach would be following these steps

### Move `eslint --init` related files to a separate repo

- Move the `eslint/lib/init/*` to the new repo's `lib/*` directory
- Add the following `dependencies` in `package.json`(the last 3 dependencies will be removed in next step)
- Add the following `dependencies` in `package.json`(the last 2 dependencies will be removed in next step)

- `enquirer`
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
- `progress`
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -42,7 +42,7 @@ The approach would be following these steps
- `json-stable-stringify-without-jsonify`
- `cross-spawn`
- `debug`
- `@eslint/eslintrc` (to be removed)
- `@eslint/eslintrc`
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
- `eslint` (to be removed)
- `espree` (to be removed)

Expand All @@ -57,15 +57,15 @@ The approach would be following these steps
- `shelljs`

It was almost done: [aladdin-add/eslint-create](https://github.com/aladdin-add/eslint-create).
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
I will transfer the repo to the eslint group later.
I will make a PR once the official repo is created later.

### Remove eslint auto-config

- Remove auto-config(`eslint/lib/init/autoconfig.js`), and its tests(`eslint/tests/lib/init/autoconfig.js`)
btmills marked this conversation as resolved.
Show resolved Hide resolved

### Update eslint cli usage

- Whenever `--init` command is being used, show a warning(e.g. "You can also run this command directly using 'npm init @eslint'") and run `npx @eslint/create` using `child_process` of the native node modules.
- 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.
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for more opinions first. @nzakas, @btmills what do you think about this?

Copy link
Member

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?

Copy link
Member Author

@aladdin-add aladdin-add Jun 7, 2021

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/...).

Copy link
Member Author

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. :)

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.



## Documentation
Expand All @@ -75,7 +75,7 @@ I will transfer the repo to the eslint group later.
on the ESLint blog to explain the motivation?
-->

- Documentation for `@eslint/create` will be created with proper usage and each prompt's details. In the main documentation, a link to `@eslint/create` will be given [here](https://github.com/eslint/eslint/blob/master/docs/user-guide/command-line-interface.md#--init) and basic usage and the package details will be documented.
- Documentation for `@eslint/create-config` will be created with proper usage and each prompt's details. In the main documentation, a link to `@eslint/create-config` will be given [here](https://github.com/eslint/eslint/blob/master/docs/user-guide/command-line-interface.md#--init) and basic usage and the package details will be documented.
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
Also in the cli command options, [here](https://github.com/eslint/eslint/blob/master/docs/user-guide/command-line-interface.md#options), for `--init` we need to change the description.
- It is not a breaking change, so a formal announcement in not needed.

Expand All @@ -93,7 +93,7 @@ I will transfer the repo to the eslint group later.
-->

- This might increase the maintenance burden as this will add one more repo in the organization.
- We may need to face challenges like re-directing of issues from `@eslint/create` repo to `eslint` and vice-versa.
- We may need to face challenges like re-directing of issues from `@eslint/create-config` repo to `eslint` and vice-versa.

## Backwards Compatibility Analysis

Expand Down Expand Up @@ -130,10 +130,7 @@ The current implementation in `eslint` repo is the alternative. No changes neede
you can remove this section.
-->

- Do we want to implement this as monorepo under `eslint` repo or a separate repo `@eslint/create` but github will make it as `eslint-create` or similar ?
- Do we want to implement this as monorepo under `eslint` repo or a separate repo `@eslint/create-config` but github will make it as `create-config` or similar ?

We decided not to go in the monorepo direction for this.

- What should be the name of the package/repo?

We got a consensus on @eslint/create and eslint-create.