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

fix: List optional peer dependencies #1626

Merged
merged 1 commit into from Oct 23, 2019
Merged

fix: List optional peer dependencies #1626

merged 1 commit into from Oct 23, 2019

Conversation

@eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Sep 6, 2019

Listing packages mentioned in https://github.com/websockets/ws#opt-in-for-performance-and-spec-compliance as optional peer dependencies.

Additional context:

I'm currently not using this feature myself until node bundles npm@6.11. Just creating this PR now so that we can discuss any concerns you have.

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 8, 2019

Hmm, what happens wit yarn < 1.13.0 and npm < 6.11.0. I guess peerDependenciesMeta is just ignored right? So bufferutil and utf-8-validate would be treated as normal peer dependencies right?

@eps1lon
Copy link
Contributor Author

@eps1lon eps1lon commented Sep 10, 2019

So bufferutil and utf-8-validate would be treated as normal peer dependencies right?

Yep. For those package managers it would create warnings. However, this should be preferred over creating incomplete dependency trees.

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 10, 2019

I think it makes more sense to add them as optionalDependencies if build failures do not cause installation to fail as per description. See discussion in #1220.

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 10, 2019

The main difference is that peer dependencies are not installed by default which is nice. I like optional peer dependencies. The only thing I don't like is the warning that is raised with older versions of npm/yarn.

@eps1lon
Copy link
Contributor Author

@eps1lon eps1lon commented Sep 10, 2019

Yeah I can't help you with deciding what strategy to use since I only encountered this package as a transitive dependency.

The only thing I don't like is the warning that is raised with older versions of npm/yarn.

Heard this fairly often but IMO I'd rather have a warning (with a fix by upgrading the package manager) that does no harm instead of a dependency tree that is potentially broken. Especially with the amount of dependencies in the JS ecosystem you do not want to debug your dependency tree ever. That's what package managers are for but they require information from the packages.

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 13, 2019

I'll wait until npm@6.11 is bundled in Node.js 10.

@lpinca
lpinca approved these changes Oct 23, 2019
@lpinca lpinca merged commit 289724f into websockets:master Oct 23, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 99.915%
Details
@lpinca
Copy link
Member

@lpinca lpinca commented Oct 23, 2019

Thank you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants