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

chore: remove eslint-plugin-import from peerDeps #248

Closed
wants to merge 2 commits into from

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Oct 23, 2023

The PR removes eslint-plugin-import from the peerDependencies, assuming that the eslint-plugin-i or eslint-plugin-import is most likely already present before installing eslint-import-resolver-typescript.

I am using eslint-import-resolver-typescript with eslint-plugin-i, and I don't want npm/npm to install eslint-plugin-import from peerDependencies automatically.

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2023

⚠️ No Changeset found

Latest commit: 05f9594

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@SukkaW
Copy link
Contributor Author

SukkaW commented Oct 23, 2023

@JounQin Should this PR be included in a minor version bump or a patch one?

@JounQin
Copy link
Collaborator

JounQin commented Oct 23, 2023

eslint-plugin-i is recommended to be used as npm install -D eslint-plugin-import@npm:eslint-plugin-i@latest, this should work without peer issue AFAIK?

Can you link some documents that npm will install peer dependencies automatically?

@SukkaW
Copy link
Contributor Author

SukkaW commented Oct 23, 2023

Can you link some documents that npm will install peer dependencies automatically?

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependencies

Here I quote:

In npm versions 3 through 6, peerDependencies were not automatically installed, and would raise a warning if an invalid version of the peer dependency was found in the tree. As of npm v7, peerDependencies are installed by default.


eslint-plugin-i is recommended to be used as npm install -D eslint-plugin-import@npm:eslint-plugin-i@latest, this should work without peer issue AFAIK?

I have adapted the new ESLint Flat Configuration (which will become the default configuration format of ESLint 9), which allows install and import eslint-plugin-i directly:

https://github.com/SukkaW/eslint-config-sukka/blob/238c5ff109bde19e0e6c92ae40339d43a6338b89/packages/ts/src/index.ts#L10
https://github.com/SukkaW/eslint-config-sukka/blob/238c5ff109bde19e0e6c92ae40339d43a6338b89/packages/ts/src/index.ts#L39-L40

Also, apparently someone else is facing the same issue, see another PR: #243

@JounQin
Copy link
Collaborator

JounQin commented Oct 23, 2023

Here I quote:

If you use npm install -D eslint-plugin-import@npm:eslint-plugin-i@latest, will it still install official eslint-plugin-import anyway?

I have adapted the new ESLint Flat Configuration (which will become the default configuration format of ESLint 9), which allows install and import eslint-plugin-i directly:

I believe npm install eslint-plugin-import@npm:eslint-plugin-i@latest (as dependency) will still work if you change to use import eslint_plugin_import from 'eslint-plugin-import'; instead?

I didn't notice #243 previously, @wojtekmaj does npm install -D eslint-plugin-import@npm:eslint-plugin-i@latest solution work to you?

@wojtekmaj
Copy link

@JounQin Oh wow, didn't think about that! Yes, this works!

@SukkaW
Copy link
Contributor Author

SukkaW commented Oct 23, 2023

If you use npm install -D eslint-plugin-import@npm:eslint-plugin-i@latest, will it still install official eslint-plugin-import anyway?

Sadly yes. However, that's a different problem: eslint-plugin-next also depends on eslint-plugin-import.

@wojtekmaj
Copy link

The answer to that problem, or rather a workaround, is Yarn's resolutions. I believe other package managers have similar features too.

@JounQin
Copy link
Collaborator

JounQin commented Oct 23, 2023

Sadly yes. However, that's a different problem: eslint-plugin-next also depends on eslint-plugin-import.

So this PR can't resolve such problems, right?

@SukkaW
Copy link
Contributor Author

SukkaW commented Oct 23, 2023

Sadly yes. However, that's a different problem: eslint-plugin-next also depends on eslint-plugin-import.

So this PR can't resolve such problems, right?

Well I still want to install eslint-plugin-i directly without alias.

@JounQin
Copy link
Collaborator

JounQin commented Oct 23, 2023

Well I still want to install eslint-plugin-i directly without alias.

I personally still recommend to use alias instead, because it is an alias. And as you described when using with other config/plugins together, eslint-plugin-import and eslint-plugin-i will be installed together which is a waste. Besides, #243 would be more suitable even if we want to use eslint-plugin-i directly although I don't recommend.

@JounQin
Copy link
Collaborator

JounQin commented Nov 16, 2023

Close due to inactive

@SukkaW
Copy link
Contributor Author

SukkaW commented Nov 30, 2023

I personally still recommend to use alias instead, because it is an alias.

Now installing eslint-plugin-i as an alias introduces way too many problems, the most noticeably one is:

Oops! Something went wrong! :(

ESLint: 8.54.0

TypeError: Cannot redefine plugin "import".

@JounQin
Copy link
Collaborator

JounQin commented Nov 30, 2023

It seems you're importing i and import at the same time. Maybe you can provide a reproduction.

@SukkaW
Copy link
Contributor Author

SukkaW commented Nov 30, 2023

It seems you're importing i and import at the same time. Maybe you can provide a reproduction.

If there is a simple minimum reproduction, I would have provided it already, but there would always be duplicated eslint-plugin-i and eslint-plugin-import in my node_modules thanks to the peerDependencies and transitive dependencies.

@JounQin
Copy link
Collaborator

JounQin commented Nov 30, 2023

Can you use overrides then?

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

Successfully merging this pull request may close these issues.

None yet

3 participants