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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix customPath option in WindowsToaster constructor #382

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

yoavain
Copy link
Contributor

@yoavain yoavain commented Nov 12, 2021

Fix #377

This PR #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

* Change test to set customPath in WindowsToaster constructor rather then notification message
@yoavain
Copy link
Contributor Author

yoavain commented Nov 12, 2021

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

@yoavain
Copy link
Contributor Author

yoavain commented Jan 26, 2022

Any update on this one?

@Araxeus
Copy link
Contributor

Araxeus commented Jan 31, 2022

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
Copy link
Contributor Author

yoavain commented Feb 1, 2022

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
Copy link
Contributor

Araxeus commented Feb 1, 2022

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
Copy link
Contributor Author

yoavain commented Feb 1, 2022

Opened this PR:
#392

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

Successfully merging this pull request may close these issues.

Doesn't work on nexe
3 participants