feat: make node-notifier an optional dependency

[everett1992]

Summary

notify is an optional feature that is disabled by default. Its implementation requires node-notifier, which distributes snoreToast, a LGPL-3.0 executable. node-notifier also includes terminal-notifier and notifu which are not MIT licensed.

My employer will not import node-notifier to our npm repository due to the LGPL executable which means we cannot use jest.

This pull request make node-notifier an optional dependency, so jest install will not fail if node-notifier cannot be found. jest will throw an error if notifications are enabled and node-notifier cannot be found.

Test plan

Tested by running jest --notify after removing node-notifier from node_modules.

jest errors with 'notify reporter requires optional dependency node-notifier but it was not found'

Tested by running jest after removing node-notifier

jest completes successfully

Tested by running jest --notify without removing node-notifier.

jest completes successfully with notifications.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[codecov-io]

Codecov Report

Merging #8918 into master will increase coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #8918      +/-   ##
==========================================
+ Coverage   64.23%   64.25%   +0.01%     
==========================================
  Files         276      276              
  Lines       11700    11706       +6     
  Branches     2864     2865       +1     
==========================================
+ Hits         7516     7522       +6     
  Misses       3552     3552              
  Partials      632      632

Impacted Files	Coverage Δ
packages/jest-reporters/src/notify_reporter.ts	75% <88.88%> (+3.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 736edd2...3cd3ee6. Read the comment docs.

[SimenB]

This works for me. Doesn't seem like it'll affect most users, so happy to make the change so you can adopt Jest.

Could you add a unit test? I haven't tried before, but I assume you can throw inside of the factory of jest.mock.

Also, please update the changelog and sign the CLA

[everett1992]

• Amazon has a corporate relationship with Facebook, I'm waiting on an internal response to the CLA
• What kind of changelog update makes sense to you? I think it's a fix.

[SimenB]

What kind of changelog update makes sense to you? I think it's a fix.

Yeah, I think so

[SimenB]

If we can avoid the duplicated types, I think this is good to go!

[everett1992]

The node-notifier typedefs are nasty so I switched to loadNotifier returning typeof import('node-notifier').

loadNotifier is almost a generic require-with-nice-message-on-optional-dependency except the type is typeof import('node-notifier').

Typescript doesn't allow typeof import(generic), it must have a string literal.

// error :(
function requireOptional<M extends string>(path: M): typeof import(M) | null {
  try {
    return require(path);
  } catch (err) {
    return null;
  }
}

I created an issue against Typescript asking for this feature a while ago with a different usecase.
microsoft/TypeScript#32705

[SimenB]

Awesome, thanks! Just need the CLA and we can merge

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[millette]

I just upgraded to 25.2.0 (from 25.1.0) where in my config notify: true. I'm not seeing notifications anymore.

npm ls node-notifier shows it's installed under @jest/[email protected] but I also installed it manually in my project. Still, no notifications, even when I jest --notify.

Details: #9701

[SimenB]

This change was in 25.1.0 as well. Could be #9567? Could you revert that inside node_modules manually?
Regardless, please open up a separate issue with a reproduction 🙂

[winnemucca]

I am reading through the conversation about node-notifier. To be clear the optional flag was added in 25.1.0? With notify: false I will no longer have a dependency on node-notifier? This will also not be included in my node_modules?

[github-actions]

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.