fix: app.relaunch loses args when execPath specified
I think this should fix #33686, but I didn't have a local toolchain to do the full build/test. However, CircleCI was nice enough to build this PR for me, and I tested the resulting Electron artifact using my gist from 33686, and it fixes the issue for me!
Description of Change
The change from bitwise to boolean "or"-operator in 1e37162 caused a short-circuit of the evaluation of a function-call with side-effects. This change pre-evaluates the functions and assigns the results to separate local variables to ensure the side-effects apply in all cases, then combines the variables with the short-circuit-able boolean operator in the conditional.
Checklist
- [x] PR description included and stakeholders cc'd
- [x]
npm testpasses - [ ] tests are changed or added
- [ ] relevant documentation is changed or added
- [x] PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
Release Notes
Notes: Fixes #33686
💖 Thanks for opening this pull request! 💖
We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.
Examples of commit messages with semantic prefixes:
-
fix: don't overwrite prevent_default if default wasn't prevented -
feat: add app.isPackaged() method -
docs: app.isDefaultProtocolClient is now available on Linux
Things that will help get your PR across the finish line:
- Follow the JavaScript, C++, and Python coding style.
- Run
npm run lintlocally to catch formatting errors earlier. - Document any user-facing changes you've made following the documentation styleguide.
- Include tests when adding/changing behavior.
- Include screenshots and animated GIFs whenever possible.
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
I don't have a local toolchain to do the full build/test. Could someone please check this for me?
Maybe you could write a test with your gist and we'll know if your fix works if it passes on CI?
@p120ph37 please fill out the PR template you removed in order for us to continue reviewing this PR.
@RaisinTen , I wasn't able to test initially, because I don't have a working build toolchain, but after filing this PR, I was able to use the CircleCI build output to test using my gist, and it solves the issue for me.
@codebytere , now that I've been able to actually test this code-change with the CircleCI output after the initial skeleton PR, I've filled in the full PR template.
I wasn't able to test initially, because I don't have a working build toolchain, but after filing this PR, I was able to use the CircleCI build output to test using my gist, and it solves the issue for me.
That's good to know :+1:. A test would still be beneficial though. It would prevent further regressions in the future.
Congrats on merging your first pull request! 🎉🎉🎉
Release Notes Persisted
Fixed an issue where app.relaunch loses args when execPath is specified.
I have automatically backported this PR to "19-x-y", please check out #35252
I have automatically backported this PR to "20-x-y", please check out #35253
I have automatically backported this PR to "21-x-y", please check out #35254