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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: make node-notifier a peer dep #10977

Merged
merged 4 commits into from Dec 24, 2020
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 23, 2020

Summary

node-notifier is 5MB due to the vendored binaries.

$ du -d1 -h node_modules/node-notifier
256K    node_modules/node-notifier/node_modules
 20K    node_modules/node-notifier/lib
 24K    node_modules/node-notifier/notifiers
5.4M    node_modules/node-notifier/vendor
5.7M    node_modules/node-notifier

This is the single biggest directory in node_modules of a fresh Jest install, beating out the combined @babel directory (albeit barely)

$ du -d1 -h node_modules/ | sort -hr | head
 51M    node_modules/
5.7M    node_modules//node-notifier
5.7M    node_modules//@babel
4.8M    node_modules//lodash
3.8M    node_modules//jsdom
1.2M    node_modules//acorn
1.2M    node_modules//@types
1.1M    node_modules//ajv
984K    node_modules//@jest
960K    node_modules//snapdragon

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

馃

"optionalDependencies": {
"node-notifier": "^8.0.0"
"peerDependencies": {
"node-notifier": "^8.0.1 || ^9.0.0"
Copy link
Member Author

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

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

馃憤馃徏

packages/jest-reporters/src/NotifyReporter.ts Outdated Show resolved Hide resolved
SimenB and others added 2 commits December 24, 2020 13:03
Co-authored-by: Micha艂 Pierzcha艂a <thymikee@gmail.com>
@merceyz
Copy link
Contributor

merceyz commented Dec 24, 2020

Hopefully the peer dep "bubbles" up so it's enough that the end user adds the dep also in PnP

It wont bubble up to the user, it will probably work because of the fallback pool but turning that off will stop it

@SimenB
Copy link
Member Author

SimenB commented Dec 24, 2020

Hopefully the peer dep "bubbles" up so it's enough that the end user adds the dep also in PnP

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 jest?

@merceyz
Copy link
Contributor

merceyz commented Dec 24, 2020

Yes, there have been talks about having them bubble up but nothing has been decided yet.
There is also yarnpkg/berry#3 but that still requires declaring them

@SimenB SimenB merged commit 4102243 into jestjs:master Dec 24, 2020
@SimenB SimenB deleted the notifier-peer-dep branch December 24, 2020 14:43
@vkrizan
Copy link

vkrizan commented Feb 4, 2021

Hi, would it be possible to release this patch? Thank you.

@SimenB
Copy link
Member Author

SimenB commented Feb 4, 2021

It will be part of Jest 27

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants