Skip to content

Conversation

@nzakas
Copy link
Member

@nzakas nzakas commented Sep 2, 2024

Prerequisites checklist

What is the purpose of this pull request?

Make ESLint an optional peer dependency.

What changes did you make? (Give an overview)

Added peerDependenciesMeta to package.json in order to set eslint as an optional peer dependency.

Practically speaking, the plugin doesn't require ESLint to be useful. We are using it in the Code Explorer without ESLint, where it's installation is causing npm errors because Code Explorer uses ESLint v8.x.

Related Issues

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

It seems like we shouldn't block installation of this package even if ESLint isn't installed or ESLint v8.x is installed, but let me know if anyone has a different opinion. From what I can tell, this change won't have any noticeable effect to most users of this plugin.

@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Sep 2, 2024
Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

Marking ESLint as optional is a good idea. LGTM

@fasttime
Copy link
Member

fasttime commented Sep 2, 2024

It seems like we shouldn't block installation of this package even if ESLint isn't installed or ESLint v8.x is installed, but let me know if anyone has a different opinion.

I think that to allow installing ESLint v8.x without the --force flag, the peerDependencies version for eslint should be changed to something like "^8.0.0 || ^9.6.0". Currently it's "^9.6.0".

"eslint": "^9.6.0",

Making the peer dependency optional will allow a package to install @eslint/json without eslint, but not with a version of eslint which is out of range.

For the same reason in eslint we have the jiti optional peer dependecy version set to "*" and not "^1.21.0", so consumers that use jiti for other purposes than to support eslint are free to install any version they want.

From what I can tell, this change won't have any noticeable effect to most users of this plugin.

Probably most users of this plugin will already have a new versioneslint declared in their devDependencies. So yes, there shouldn't be any noticeable effects. I'm fine with merging this as a fix: 👍

@nzakas
Copy link
Member Author

nzakas commented Sep 3, 2024

Making the peer dependency optional will allow a package to install @eslint/json without eslint, but not with a version of eslint which is out of range.

Hmm, not what we want. I wonder if it's best just remove the peer dependency altogether?

@fasttime
Copy link
Member

fasttime commented Sep 3, 2024

Hmm, not what we want. I wonder if it's best just remove the peer dependency altogether?

Makes sense I think, if it should be possible to install this package irrespective of eslint.

Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@fasttime fasttime 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants