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: expose cli entrance #17690

Closed
wants to merge 1 commit into from

Conversation

zanminkian
Copy link

@zanminkian zanminkian commented Oct 28, 2023

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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

Expose cli entrance. I have a cli tool want to reuse the eslint's cli entrance by using:

// ./bin/my-lint.js

import('eslint/bin')

See the similar PR for lint-staged.

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

No.

@zanminkian zanminkian requested a review from a team as a code owner October 28, 2023 21:16
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Oct 28, 2023
@netlify
Copy link

netlify bot commented Oct 28, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 26baf1c
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/653d7a257ed2700008e3eb9f

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: zanminkian / name: 曾明健 (26baf1c)

@nzakas
Copy link
Member

nzakas commented Oct 30, 2023

I'm not sure I understand why this would be helpful. The CLI entrance doesn't export anything, so you'd just be running the code without any arguments, which would fail. What is it you're trying to achieve that can't be done via exec("npx eslint .")?

@zanminkian
Copy link
Author

The CLI entrance doesn't export anything, so you'd just be running the code without any arguments, which would fail.

Other tools based on eslint can easily hook the cli.

// ./bin/my-lint.js
process.argv.push('--fix') // always fix

import('eslint/bin')
my-lint .

What is it you're trying to achieve that can't be done via exec("npx eslint .")?

I have an npm package (a cli tool) based on eslint. If I use exec("npx eslint .") in my npm package, users who use my tool via npx my-lint will be asked to install eslint again, which is not friendly. If I use process.argv.push('--fix'); import('eslint/bin'), user will not be asked to install eslint.

@nzakas
Copy link
Member

nzakas commented Oct 31, 2023

If I use exec("npx eslint .") in my npm package, users who use my tool via npx my-lint will be asked to install eslint again, which is not friendly.

I don't understand this. If your tool is based on ESLint, then isn't ESLint already installed? Why would they be asked to install it again?

@nzakas
Copy link
Member

nzakas commented Oct 31, 2023

And note: You'll need to sign the CLA for us to consider this PR.

@zanminkian
Copy link
Author

If your tool is based on ESLint, then isn't ESLint already installed? Why would they be asked to install it again?

This will appear when user do not install my-lint into package.json and just want to run npx my-lint for a quick test, because of exec("npx eslint .") in my-lint

@nzakas
Copy link
Member

nzakas commented Nov 1, 2023

I see. And what is the reason you don't use the ESLint class?

@zanminkian
Copy link
Author

And what is the reason you don't use the ESLint class?

Because it's hard to use, and I have to rewrite the same code as ./bin/eslint of eslint. Let's say I have a cli tool to run prettier and eslint, and this tool have the same cli interface with prettier and eslint.

// ./bin/my-lint.js
const origin = process.argv
process.argv = origin.filter(i=>!i.startsWith('--eslint')).map(i=>i.replace('--prettier', '')))
await import('prettier/bin')
process.argv = origin.filter(i=>!i.startsWith('--prettier')).map(i=>i.replace('--eslint', '')))
await import('eslint/bin')
my-lint . --eslint-f --prettier-w

In this example, user don't need to learn new api. And I, who created it, don't need to rewrite the same code as ./bin/eslint.js of eslint.

@nzakas
Copy link
Member

nzakas commented Nov 6, 2023

I see, thanks for explaining.

I guess I'm still having a hard time understanding this use case. If your tool is bundling ESLint as a dependency, then you should be able to run it via exec() directly. It seems like using import() is jumping through a lot of unnecessary hoops. What am I missing?

@zanminkian
Copy link
Author

zanminkian commented Nov 7, 2023

Hi.

  1. Using exec() is unfriendly to user. They have to install eslint again. I think I've explain here. My tool is git-validator. You can
    1. create a empty folder
    2. and then run npm init -y to create a new package.json
    3. and then run npx git-validator. You will find that you need to install eslint again. See this line
      image
  2. Pnpm hoist eslint by default. If Pnpm do not hoist eslint by default in the future, using exec('eslint . --fix') will always fail. See https://pnpm.io/npmrc#public-hoist-pattern.
  3. If you have time, please see my source code here and the PR. I'm not sure that I have explained completely. If you need more info, please let me know. I am glad to provide it.

@nzakas
Copy link
Member

nzakas commented Nov 7, 2023

Using exec() is unfriendly to user. They have to install eslint again.

This is the part I don't understand. If you have a dependency on eslint, then it should be installed, and you should be able to execute it. I wrote about an approach that would seem to solve this problem by just modifying the PATH you pass to exec().

In general, I just don't like adding things to the public interface to support an edge case for one implementation. That's why I'm asking so many questions. This was a use case we didn't intend to support, so we need a solid justification for adding it.

@zanminkian
Copy link
Author

zanminkian commented Nov 10, 2023

Hi @nzakas. Following your suggection, it can be executed. But the code will be a little bit complex. Comparing with the implementaions about prettier, lint-staged and commitlint, integrating eslint one is the most complicated. Any one of the others is only one line of code. All of them can do it. Why cann't eslint? 🤔

@nzakas
Copy link
Member

nzakas commented Nov 13, 2023

See, you now have a script that will work for all CLIs regardless of whether or not the package.json exports a bin. This is a much more reliable way of working with any CLIs, as opposed to continuing to ask every CLI you use to update their package.json. I'd suggest you make that your preferred way of working with CLIs and continue to tweak the approach to make it easier for yourself. I don't think 14 lines of code to universally work with any CLI in the Node.js ecosystem is too complex.

@nzakas nzakas closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

2 participants