-
-
Notifications
You must be signed in to change notification settings - Fork 70
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: move init command to separate package #58
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
- Repo: [eslint/eslint](https://github.com/eslint/eslint) | ||
- Start Date: 2020-06-29 | ||
- RFC PR: | ||
- Authors: Aniketh Saha ([anikethsaha](https://github.com/anikethsaha)) | ||
|
||
# Move --init flag into a separate utility | ||
|
||
## Summary | ||
|
||
Move the `init` command from main repo ([eslint](https://github.com/eslint/eslint)) to a new repo named `@eslint/create` | ||
|
||
## 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. | ||
|
||
## Detailed Design | ||
|
||
<!-- | ||
This is the bulk of the RFC. | ||
|
||
Explain the design with enough detail that someone familiar with ESLint | ||
can implement it by reading this document. Please get into specifics | ||
of your approach, corner cases, and examples of how the change will be | ||
used. Be sure to define any new terms in this section. | ||
--> | ||
|
||
The implementation is mainly migrating the code from main repo to `@eslint/create`. As far as the modules are concerned, all shared modules are being shipped currently with the eslint so we can use those modules through eslint itself. | ||
|
||
The approach would be following these steps | ||
|
||
- Move the `eslint/lib/init/*` to the new repo's `src/*` directory | ||
- Add the following `dependencies` in `package.json` | ||
- `enquirer` | ||
- `progress` | ||
- `semver` | ||
- `espree` (peer) | ||
- `lodash` | ||
- `json-stable-stringify-without-jsonify` | ||
- `cross-spawn` | ||
- `eslint` (peer) | ||
- `debug` (peer) | ||
- for all those modules coming inside `../**/*`, it would be replaced using `eslint/**/*` | ||
- For `tests`, move the `tests/lib/init` to new repo's `tests/init` | ||
- Add the following `devDependencies` in `package.json` | ||
- `chai` | ||
- `sinon` | ||
- `js-yaml` | ||
- `proxyquire` | ||
- `shelljs` | ||
- create `tests/utils` and add the following functions | ||
- [`defineInMemoryFs`](https://github.com/eslint/eslint/blob/6677180495e16a02d150d0552e7e5d5f6b77fcc5/tests/_utils/in-memory-fs.js#L250) | ||
- for `conf/*`, it is not a part of the public API and it cant be accessed from `eslint` module, we need a keep these synced with the main repo (`eslint`). We can choose from the following approach to solve this : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on what we need this for? Is the data we need from here something we could calculate on the fly somehow? I don't think the extra calculation cost is too big a deal since this is intended to be run as a one and done script. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also confused by this. I don't think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is using it like here so I think we need to move this to the new repo right ? May be I am missing something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another auto config dependency. I think we should get rid of auto config to simplify things. |
||
- using the `eslint-github-bot`, whenever there is a PR opened that changes the file for the `conf` directory in `eslint` repo, create an issue in the `@eslint/create` repo to have a look in the PR and if required submit a change in `@eslint/create`. | ||
- OR, while doing the release for both `eslint` and `@eslint/create` repos, the release script can be configured to copy-parse the `conf` folder from `eslint` to `@eslint/create` before the release just like the `website` repo is being updated currently. | ||
- After completion of this repo, whenever someone uses the `--init` flag, an error will be thrown stating the correct step to do the migration i.e using `npx @eslint/create` or `npm @eslint` or `yarn @eslint`. I am not suggesting to use run the `@eslint/create` module whenever `--init` command is being called because this would require to use the `@eslint/create` module as a dependency in `eslint` and in `@eslint/create` we are already being using `eslint` as a dependency, so there would be circular dependencies. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I’d say let’s just get rid of auto config altogether. I don’t think it’s that useful anymore. Plus, we can avoid a direct dependency on ESLint that way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A tool like eslint-nibble will be a much better experience than autoconfig, so I have no problem with removing it 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry but I am not getting what do you mean with auto-config ? is it the whole --init all together? or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, Thanks for the details. But still, we need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are linter and CLIEngine used outside of auto config? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no I guess, cliEngine is being used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, sounds like we are on the right track. I'd suggest updating this RFC with the latest agreements. I think we're close! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had such high hopes for autoconfig, but I agree that it was never terribly useful, partly because it only worked if you were already strictly following a convention. I'm actually surprised it hasn't been removed already. :) This RFC looks like a great move. I love what y'all are doing and hope everyone is well and having a happy new year! |
||
|
||
## Documentation | ||
|
||
<!-- | ||
How will this RFC be documented? Does it need a formal announcement | ||
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 below as a deprecated notice and migration guide. | ||
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 for this with deprecated details as the command is still active just that there will be error that will recommend to use `@eslint/create` package instead. | ||
Yes as it will be a major change for the `eslint` repo itself as it is being removed from the main package/repo, I guess a blog can be a good solution to solve initial queries. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that this is a breaking change. Most people only use the command once, so we're not in danger of breaking anyone's build. I'd say this is a semver-minor change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though I have not seen any project where it is being used in the build or in the testing so yea agreed, it is a semver-minor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Provided |
||
|
||
## Drawbacks | ||
|
||
<!-- | ||
Why should we *not* do this? Consider why adding this into ESLint | ||
might not benefit the project or the community. Attempt to think | ||
about any opposing viewpoints that reviewers might bring up. | ||
|
||
Any change has potential downsides, including increased maintenance | ||
burden, incompatibility with other tools, breaking existing user | ||
experience, etc. Try to identify as many potential problems with | ||
implementing this RFC as possible. | ||
--> | ||
- 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. | ||
- If we do not add proper tooling for handling the `conf` directory in `eslint` to `@eslint/create` repo as mentioned in the [detailed design](@detailed_design)'s 7th point, there will be an overhead work on looking through each PR/change particularly to find out whether there is a need for updating the `conf` directory in `@eslint/create` or not. | ||
|
||
|
||
## Backwards Compatibility Analysis | ||
|
||
<!-- | ||
How does this change affect existing ESLint users? Will any behavior | ||
change for them? If so, how are you going to minimize the disruption | ||
to existing users? | ||
--> | ||
- Releasing with a proper message which the command is being run from `eslint` with something like - `"this command has been moved from here, use 'npx @eslint/create'"` might be a good way to address backward compatibility. | ||
- Or whenever `--init` command is being used, show a warning and run `npx @eslint/create` using `child_process` of the native node modules. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can make use of the output message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works too. We should use whatever we end up documenting in the getting started guide. |
||
- Otherwise we can keep the feature in the main repo as well for starting few days/weeks with a warning and then removing it completing with an error message but this would lead to circular dependencies and that is not an ideal case for managing dependencies, so I think we should remove this feature completely with an error message as mentioned above. | ||
|
||
## Alternatives | ||
|
||
<!-- | ||
What other designs did you consider? Why did you decide against those? | ||
|
||
This section should also include prior art, such as whether similar | ||
projects have already implemented a similar feature. | ||
--> | ||
The current implementation in `eslint` repo is the alternative. No changes needed. | ||
|
||
## Open Questions | ||
|
||
<!-- | ||
This section is optional, but is suggested for a first draft. | ||
|
||
What parts of this proposal are you unclear about? What do you | ||
need to know before you can finalize this RFC? | ||
|
||
List the questions that you'd like reviewers to focus on. When | ||
you've received the answers and updated the design to reflect them, | ||
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 ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to start going in the monorepo direction for this. We already have trouble dealing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what should be the name of that ? we cannot have it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's a big deal if the repo is called |
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 will be brought in by including
eslint
, so we don't need to include it separately.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 I added
peer
to indicate the similar.I will remove it .