-
-
Notifications
You must be signed in to change notification settings - Fork 163
fix(deps)!: add missing peerDependencies entries #630
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
base: master
Are you sure you want to change the base?
fix(deps)!: add missing peerDependencies entries #630
Conversation
BREAKING CHANGE: Adds missing `peerDependencies`. This adds `peerDependencies` entries for `@swc/core` and `esbuild` at the same versions specified in `devDependencies`. Per the [npm documentation](https://docs.npmjs.com/cli/v9/configuring-npm/package-json\#peerdependenciesmeta), the contents of `peerDependenciesMeta` will be ignored unless those packages also appear in the `peerDependencies` field. For a development environment, these two optional packages will be installed _anyway_ due to their appearance in `devDependencies`.
60a7a62
to
b67a22d
Compare
We already have it - terser-webpack-plugin/package.json Line 52 in b67a22d
|
No |
...what?
|
Sorry, no, we don't want to install extra dependencies and increase size of a package, also esbuild and
That's why they are listed in
Don't use outdated npm |
@alexander-akait I mentioned npm v6 because this package purports to support Node.js v10.13.0 or newer. Node.js v10.x, v12.x, and v14.x all ship with But the above is moot because I tested my change with npm v10.9.2 and npm v6.14.16; packages listed in Before dismissing this change, I encourage you to pull this PR and test it like so: # this will give you a tarball as would be installed from the registry and ensures
# behavior isn't influenced by dev dependencies
npm pack # (note the path to the tarball)
mkdir -p /tmp/pr630
cd /tmp/pr630
npm install /path/to/terser-webpack-plugin-5.3.14.tgz
npm ls esbuild
npm ls @swc/core
npm ls uglify-js You should see they are not installed. But Note that |
We have it only for compatibility with old version, not more, we don't want to make something more than just have it for CI purposes
This is because you test it using |
Sorry to make more demands on your time, but ... Can you please provide a reproducible example of how this would break consumers? For example, a specific version of a specific package manager where this PR introduces a breaking change. I'm not saying you're wrong, but I've tried with other versions of other package managers and have not been able to reproduce a situation in which these peer dependencies get installed by default. |
@boneskull to be honest I don't remember because it was a long time ago, your pr does everything correctly and the logic is correct, most likely it was fixed in npm in some patch versions, but I would prefer to leave it until the next major release |
@alexander-akait Thanks. That's fine. I agree that there was weird behavior in the past, but I think maybe those days have passed. How would we get this into the next major? Leave unmerged until ready? |
@boneskull just leave it, I will rebase and merge for the next major release |
BREAKING CHANGE: Adds missing
peerDependencies
.This adds
peerDependencies
entries for@swc/core
andesbuild
at the same versions specified indevDependencies
.Per the npm documentation, the contents of
peerDependenciesMeta
will be ignored unless those packages also appear in thepeerDependencies
field. For a development environment, these two optional packages will be installed anyway due to their appearance indevDependencies
.This PR contains a:
Motivation / Use-Case
I would have expected optional peer dependencies to be installed when I wanted them to be installed
Breaking Changes
This is a breaking change for users of
npm
prior to v7.0.0. Prior to v7.0.0, peer dependencies are not automatically installed by default and thepeerDependenciesMeta
field is unrecognized.Additional Info
Note that:
npm
v6.x by defaultnpm
v7.0.0 was released in 2020I would suggest adding
npm
v7.0.0 toengines
(or ideally dropping support for Node.js v10.x and 12.x), but it's a breaking change either way.