search

mikaelbr / node-notifier

A Node.js module for sending notifications on native Mac, Windows and Linux (or Growl as fallback)
MIT License
5.73k stars 322 forks source link

Fix customPath option in WindowsToaster constructor #382

Closed yoavain closed 2 years ago

yoavain commented 2 years ago

Fix https://github.com/mikaelbr/node-notifier/issues/377

This PR https://github.com/mikaelbr/node-notifier/pull/373 caused customPath set in WindowsToaster constructor to be ignored.

The fix is a one-liner, but I also added a test that test the entire flow using nexe

yoavain commented 2 years ago

My fix assumes that customPath sent in message can override the one set in the constructor

yoavain commented 2 years ago

Any update on this one?

Araxeus commented 2 years ago

image You commited package.lock

also I think the only change in the whole PR should just be

    const localNotifier = options.customPath || this.options.customPath
      (notifier + '-x' + (is64Bit ? '64' : '86') + '.exe');

and if you want open others pull request for other changes

yoavain commented 2 years ago

I committed package-lock.json because

  1. the committed one, is out of sync from package.json file
  2. I added some dev dependencies for tests.

I can leave this PR as is, and open another one, just with the one-liner fix

Araxeus commented 2 years ago

I can leave this PR as is, and open another one, just with the one-liner fix

Sounds good to me, that one should hopefully gets merged more quickly 😅

yoavain commented 2 years ago

Opened this PR: https://github.com/mikaelbr/node-notifier/pull/392