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: app.relaunch loses args when execPath specified #35108
Conversation
Commit 1e37162 changed a bitwise-or operator to a boolean-or operator, which altered the short-circuit behavior and skipped the side-effect of populating the "args" variable. This commit restores the original side-effect behavior, while still using a boolean-or operator to keep the linter happy. Addresses issue electron#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:
Things that will help get your PR across the finish line:
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. |
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. |
That's good to know 👍. A test would still be beneficial though. It would prevent further regressions in the future. |
62da63f
to
638b70b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
Congrats on merging your first pull request! 🎉🎉🎉 |
Release Notes Persisted
|
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 |
fix: app.relaunch loses args when execPath specified (electron#35108) Co-authored-by: Aaron Meriwether <me@ameriwether.com>
fix: app.relaunch loses args when execPath specified (#35108) Co-authored-by: Aaron Meriwether <me@ameriwether.com>
fix: app.relaunch loses args when execPath specified (electron#35108) Co-authored-by: Aaron Meriwether <me@ameriwether.com>
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.
Fixes #33686
Checklist
npm test
passesRelease Notes
Notes: Fixed an issue where app.relaunch loses args when execPath is specified.