Skip to content

feat: make eslint a peer dependency #1

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

marcalexiei
Copy link
Contributor

Greetings,
this PR change moves eslint to peerDependencies.

This ensures that only a version of eslint is present.
I tried this library on a project with eslint@9.13.0 and eslint@9.14.0 was installed in eslint-p node_modules folder.

@fasttime
Copy link
Collaborator

fasttime commented Nov 4, 2024

Hi @marcalexiei, the problem is that eslint-p doesn't work with all versions of ESLint v9. The current implementation will definitely error out with eslint@9.11.0 and nothing can be said about future ESLint releases. While it's true that you could run it with eslint@9.13.0, you should get slightly better performance using eslint-p@0.12.0, which includes eslint@9.13.0 as a dependency. The reason is that the new version includes an optimization that doesn't work well in eslint@9.13.0 because of eslint/eslint#19025, which is now solved.

Unfortunately, I don't have capacities to support multiple versions of ESLint in this project, although the documentation could be improved.

@marcalexiei
Copy link
Contributor Author

Thanks for the explanation,
just to check if I have understand correctly: each version of this package will work with only strict set of ESLint version?
If yes I think that we can still use the peerDependencies to express the exact version ESLint supported versions:

"peerDependencies": {
-  "eslint": "^9"
+  "eslint": ">= 9.14.0 < 9.15"
},

Consider the following scenario:

  • I have installed eslint@9.11.0 when I run npx eslint -v I get v9.11.0 which is expected
  • In the same repository if I run npx eslint-p -v I get
    eslint-p v0.13.0
    ESLint v9.14.0
    

Assuming I'm using an editor with eslint support the editor will pick 9.11.0 version.
If something is changed between those two ESLint versions (e.g.: some rule have changed their implementation)
I might end up in one of these situations:

  • get different report from running npx eslint and npx eslint-p
  • the editor reports a problem that eslint-p doesn't report or viceversa

This kind of scenario should be prevented if using peerDependencies (If I have understand peerDependencies well 😅)

What do you think?

@fasttime
Copy link
Collaborator

fasttime commented Nov 5, 2024

Thanks for the explanation, just to check if I have understand correctly: each version of this package will work with only strict set of ESLint version?

Yes, the idea is that you shouldn't need to install eslint to run eslint-p and know what version will be running. I realize that's not obvious from the docs.

If yes I think that we can still use the peerDependencies to express the exact version ESLint supported versions:

"peerDependencies": {
-  "eslint": "^9"
+  "eslint": ">= 9.14.0 < 9.15"
},

Consider the following scenario:

* I have installed `eslint@9.11.0` when I run `npx eslint -v` I get `v9.11.0` which is expected

* In the same repository if I run `npx eslint-p -v` I get
  ```
  eslint-p v0.13.0
  ESLint v9.14.0
  ```

Assuming I'm using an editor with eslint support the editor will pick 9.11.0 version. If something is changed between those two ESLint versions (e.g.: some rule have changed their implementation) I might end up in one of these situations:

* get different report from running `npx eslint` and `npx eslint-p`

* the editor reports a problem that `eslint-p` doesn't report or viceversa

This kind of scenario should be prevented if using peerDependencies (If I have understand peerDependencies well 😅)

What do you think?

npx does not check for conflicting versions in locally installed dependencies, only npm install does that. So in a scenario where a user installed eslint@9.11.0 locally and then runs npx eslint-p, there would be no error even if peerDependecies specified an unsatisfied version range for eslint.

@marcalexiei
Copy link
Contributor Author

Gotcha,
the last option I see using peerDependency is to check ESLint version against a list of supported versions after it has been imported here:

const { ESLint } = await import('eslint');

If the version isn't included in the supported list we can report a warning to the user.

If you want I can submit a PR with this change otherwise I can setup another PR updating the docs with this information.

@fasttime
Copy link
Collaborator

fasttime commented Nov 6, 2024

Thanks! Feel free to update the docs with what is missing. I'm not sure yet about reporting a warning to the user if there's a version mismatch. I see two problems:

  • Both versions of ESLint could produce the same result, and in that case the warning would only add noise to the output.
  • The detection would be error prone. We'd have to look into package.json to determine what versions of ESLint are acceptable to the user, because for example version 9.14.0 would satisfy a range like "eslint": "^9.11.0" but not "eslint": "~9.11.1". And there is no guarantee about the location of package.json or if it exists in the first place. Even IDEs like VSCode don't always get it right.

I'd suggest to reconsider this change later if users have trouble with a particular use case.

@marcalexiei
Copy link
Contributor Author

marcalexiei commented Nov 6, 2024

Thanks! Feel free to update the docs with what is missing

Sure! I'll open it in the upcoming days.


I'd suggest to reconsider this change later if users have trouble with a particular use case.

Ok, no problem.

I would like to add some thoughts for future reference about the implementation I had in mind:

Assuming we use peerDependencies only one ESLint version will be installed on user side.
To get the installed location binary we can use an API like import.meta.resolve or the ponyfill npm package import-meta-resolve.

The given path can be used to load ESLint dynamically like already done here:

const { ESLint } = await import('eslint');

After loading eslint module we can extract the exact version from ESLint class and compare it with the supported ones.
For this operation we might use semver package (added as dependency) or just string comparison (JS has a fancy numeric string compare).

Something like:

const eslintPath = await import.meta.resolve('eslint');
// check if path do not exist and issue an error
const { ESLint } = await import('eslint'); 

// get ESLint version
const installedVersion = ESLint.version;

// check version using string compare and if version is not supported issue a warning

With this method we can get the exact version of installed ESLint without to worry about comparing semver ranges and to look for user package.json


You can close the PR if you do not want to discuss this further 🙂.

@fasttime
Copy link
Collaborator

fasttime commented Nov 8, 2024

Superseded per f5e7f74 (#2).

@fasttime fasttime closed this Nov 8, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants