electron icon indicating copy to clipboard operation
electron copied to clipboard

fix: app.relaunch loses args when execPath specified

Open p120ph37 opened this issue 3 years ago • 5 comments

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

Release Notes

Notes: Fixes #33686

p120ph37 avatar Jul 27 '22 22:07 p120ph37

💖 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 lint locally 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.

welcome[bot] avatar Jul 27 '22 22:07 welcome[bot]

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?

RaisinTen avatar Jul 28 '22 07:07 RaisinTen

@p120ph37 please fill out the PR template you removed in order for us to continue reviewing this PR.

codebytere avatar Jul 28 '22 10:07 codebytere

@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.

p120ph37 avatar Jul 28 '22 16:07 p120ph37

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.

RaisinTen avatar Jul 29 '22 05:07 RaisinTen

Congrats on merging your first pull request! 🎉🎉🎉

welcome[bot] avatar Aug 08 '22 08:08 welcome[bot]

Release Notes Persisted

Fixed an issue where app.relaunch loses args when execPath is specified.

release-clerk[bot] avatar Aug 08 '22 08:08 release-clerk[bot]

I have automatically backported this PR to "19-x-y", please check out #35252

trop[bot] avatar Aug 08 '22 08:08 trop[bot]

I have automatically backported this PR to "20-x-y", please check out #35253

trop[bot] avatar Aug 08 '22 08:08 trop[bot]

I have automatically backported this PR to "21-x-y", please check out #35254

trop[bot] avatar Aug 08 '22 08:08 trop[bot]