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

feat: move eslint --init to @eslint/create-config #15150

Merged
merged 5 commits into from Jan 9, 2022

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Oct 11, 2021

TODOs:

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

fixes #14768

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Oct 11, 2021
@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface and removed triage An ESLint team member will look at this issue soon labels Oct 11, 2021
bin/eslint.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

I think the files should be copied to the new repo https://github.com/eslint/create-config, as we decided not to go in the monorepo direction for this.

@aladdin-add
Copy link
Member Author

absolutely right -- that's why the PR is a draft, will do later. 😄

@aladdin-add
Copy link
Member Author

update: I was planning to move proxyrequire to testdouble in eslint/create-config, as proxyrequire does not support esm mocking.

@mdjermanovic mdjermanovic changed the title Update: move eslint --init to @eslint/create-config feat: move eslint --init to @eslint/create-config Oct 26, 2021
@nzakas
Copy link
Member

nzakas commented Nov 4, 2021

What’s the status on this? We are getting a bunch of issues/PRs related to —init and we need to make some progress.

@aladdin-add
Copy link
Member Author

aladdin-add commented Nov 5, 2021

Now I'm using the native Node.js esm as sindresorhus' blog (not pushed yet):

  • Add "type": "module", to package.json (below the author field)
  • Add "exports": "./index.js", to package.json (below the type field)
  • Update engines field in package.json to Node.js 12.
  • Update index.d.ts to use ESM imports/exports.
  • Update code examples in readme and index.d.ts to use ESM and also top-level await.
  • Remove 'use strict';.
  • Convert '.' to './index.js'.
  • Use the node: protocol for imports.

additionally, I replaced proxyquire to testdouble(as proxyquire does not support esm mocking).

does it make sense? Do we want to add the cjs export in the new package, too?

@mdjermanovic
Copy link
Member

  • Add "exports": "./index.js", to package.json (below the type field)
    Do we want to add the cjs export in the new package, too?

Do we want to export anything from this package? If it's meant to be only a CLI app, I think we can publish ESM-only package, without exports.

@nzakas
Copy link
Member

nzakas commented Nov 6, 2021

I agree. I don’t think we need to export anything.

@aladdin-add aladdin-add force-pushed the update/rm-eslint-init branch 2 times, most recently from 7f69c98 to a4ac601 Compare November 19, 2021 09:53
@nzakas
Copy link
Member

nzakas commented Nov 25, 2021

Where are we on this? Is there a reason all the work is being done here instead of on the new repo?

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 1, 2021
@aladdin-add aladdin-add force-pushed the update/rm-eslint-init branch 4 times, most recently from 128c2e5 to f09f525 Compare December 6, 2021 05:36
@aladdin-add
Copy link
Member Author

this is ready for review, but set to a draft to make sure we don't merge it before @eslint/create-config is released.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I was able to test this manually locally with this setup:

  1. In my eslint/create-config checkout, npm link.
  2. In my eslint/eslint checkout on this branch, npm link @eslint/create-config.
  3. In my eslint/eslint checkout on this branch, npx eslint --init.

bin/eslint.js Outdated Show resolved Hide resolved
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving as comment instead of approval pending release of @eslint/create-config.

@aladdin-add aladdin-add marked this pull request as ready for review December 18, 2021 06:06
@eslint-github-bot
Copy link

Hi @aladdin-add!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@nzakas
Copy link
Member

nzakas commented Jan 7, 2022

Where are we on this? What’s needed now?

@aladdin-add
Copy link
Member Author

The only thing is: eslint/create-config#10.

Given npx will always use latest, seems this one can even be merged first.

@nzakas
Copy link
Member

nzakas commented Jan 7, 2022

Okay, just releases v0.1.2. See my note on the issue about the version number.

@mdjermanovic
Copy link
Member

Should we keep unit tests for the config-rule.js module?

@mdjermanovic
Copy link
Member

Can we delete tests/fixtures/autoconfig/*?

aladdin-add and others added 4 commits January 8, 2022 20:02
fixes eslint#14768, fixes eslint#15159

Update: rm eslint auto config

refs: https://github.com/aladdin-add/rfcs/blob/bdc12aa062750d837e5a3bbbf2f6e5e3a98da388/designs/2021-init-command-eslint-cli/README.md#1-remove-eslint-auto-config

Update: mv eslint --init to another package

moved `lib/init/config-rule` to tools, as it is also used by some others

refs: https://github.com/aladdin-add/rfcs/blob/bdc12aa062750d837e5a3bbbf2f6e5e3a98da388/designs/2021-init-command-eslint-cli/README.md#2-move-eslint---init-related-files-to-a-separate-repo

chore: fix imports to make test passing

todo: use the new eslint api

fix: use the new eslint api (async)

fix: remove espree from deps

chore: fix a failing test

fix: a failing test

chore: cleanup TODOs

fix: allow to use local-installed eslint

wip: fix one-var

chore: lib => esm

chore: tests => esm

todo: proxyquire => td

chore: update deps to latest

fix: should write a file through fs when a ${fileType} path is passed

replaced proxyquire & sinon => td

fix: should include a newline character at EOF

chore: add testdouble

--wip-- [skip ci]

chore: remove package @eslint/create-eslint

feat: update npm --init to run `npm init @eslint/config`

docs: update getting-started

Update README.md

Update getting-started.md

chore: rm init fixtures

fix: `npm init @eslint/config` output

chore: rm unused files

chore: rm unused deps

Update bin/eslint.js

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>

chore: fix typo
@aladdin-add
Copy link
Member Author

@mdjermanovic should be addressed now. 😄

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM! Leaving open to allow others to follow up their reviews.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 0d2b9a6 into eslint:main Jan 9, 2022
@aladdin-add aladdin-add deleted the update/rm-eslint-init branch January 9, 2022 10:49
@cek333 cek333 mentioned this pull request Jan 12, 2022
1 task
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jul 9, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion cli Relates to ESLint's command-line interface feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update: separate eslint --init
4 participants