Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

Fix configuration for node-notifier #1021

Merged
merged 4 commits into from
Mar 2, 2021
Merged

Fix configuration for node-notifier #1021

merged 4 commits into from
Mar 2, 2021

Conversation

GerkinDev
Copy link
Contributor

Update configuration for node-notifier@>=6.0.0 (latest being 9.0.0, and this PR also fixes shipping with those latest versions).
See changes in mikaelbr/node-notifier@54ddb7f.
As mentioned in npm documentation, peer dependencies are no longer installed by default. So I used it to constrain the supported version of node-notifier

Update configuration for node-notifier@>6.0.0.
See changes in mikaelbr/node-notifier@54ddb7f
@cmfcmf
Copy link

cmfcmf commented Jan 5, 2021

Hey,
adding a peer dependency will emit a warning for everyone that does not use node-notifier. To prevent that, you should mark the peer dependency as optional as described here:

@cmfcmf
Copy link

cmfcmf commented Jan 8, 2021

Hmm, I don't think adding node-notifier to optionalDependencies is the right move. This will install node-notifier for everyone that uses pkg. Instead, I think it would be better to add it as an optional peer dependency like I described above (that means that the node-notifier goes into peerDependencies and peerDependenciesMeta:

{
  "peerDependencies": {
    "node-notifier": ">=6.0.0"
  },
  "peerDependenciesMeta": {
    "node-notifier": {
      "optional": true
    }
  }
}

@GerkinDev
Copy link
Contributor Author

Oh yeah my bad, I'm just learning that right now, thanks ! It will be useful for other packages ;) gonna fix this

@ThatOneCalculator
Copy link

Any update on this PR?

Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@leerob leerob merged commit d33b1c5 into vercel:master Mar 2, 2021
@SrBrahma
Copy link

I still have errors, as mentioned in mikaelbr/node-notifier#220 (comment). How can I fix it?

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Jun 16, 2021

@SrBrahma please refer to version ranges specified in https://github.com/vercel/pkg/blob/master/package.json#L61 : make sure that node-notifier@>=9.0.1 is installed, along with pkg@>4.4.9

@AliSawari
Copy link

@GerkinDev I have installed node-notifier and pkg versions correctly, but the problem still persists,
OS: Windows 10
Node: 16.15.1
output:

  The file must be distributed with executable as %2.
  %1: node_modules\node-notifier\vendor\notifu\notifu.exe
  %2: path-to-executable/notifier/notifu.exe
> Warning Cannot include file %1 into executable.
  The file must be distributed with executable as %2.
  %1: node_modules\node-notifier\vendor\notifu\notifu64.exe
  %2: path-to-executable/notifier/notifu64.exe
> Warning Cannot include file %1 into executable.
  The file must be distributed with executable as %2.
  %1: node_modules\node-notifier\vendor\terminal-notifier.app\Contents\MacOS\terminal-notifier
  %2: path-to-executable/notifier/terminal-notifier
> Warning Cannot include file %1 into executable.
  The file must be distributed with executable as %2.
  %1: node_modules\node-notifier\vendor\snoreToast\snoretoast-x64.exe
  %2: path-to-executable/notifier/snoretoast-x64.exe
> Warning Cannot include file %1 into executable.
  The file must be distributed with executable as %2.
  %1: node_modules\node-notifier\vendor\snoreToast\snoretoast-x86.exe
  %2: path-to-executable/notifier/snoretoast-x86.exe

@GerkinDev
Copy link
Contributor Author

@AliSawari i think that since this PR has been merged, you should open a new issue or search for an already & still open one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants