-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: make node-notifier a peer dep #10977
Conversation
"optionalDependencies": { | ||
"node-notifier": "^8.0.0" | ||
"peerDependencies": { | ||
"node-notifier": "^8.0.1 || ^9.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8.0.1
to include the security fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
It wont bubble up to the user, it will probably work because of the fallback pool but turning that off will stop it |
So for this to work properly I'll have to add the peer dependency stanza to all packages between the one that uses it and |
Yes, there have been talks about having them bubble up but nothing has been decided yet. |
Hi, would it be possible to release this patch? Thank you. |
It will be part of Jest 27 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
node-notifier
is 5MB due to the vendored binaries.This is the single biggest directory in
node_modules
of a fresh Jest install, beating out the combined@babel
directory (albeit barely)I think this should work. Hopefully the peer dep "bubbles" up so it's enough that the end user adds the dep also in PnP.
node-notifier
is already optional (via #8918), so the code change here is pretty trivial.Test plan
🤞