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

Add wait: false to options object for opn #5821

Merged
merged 1 commit into from Mar 15, 2019

Conversation

@evalexpr
Copy link
Contributor

commented Nov 15, 2018

Fixes #3113.

As far as I can see, there is no downside to adding { wait: false } to the option args when invoking opn, if someone can think of one then I'm happy to test that path.

Adding wait : false adds cpOpts.stdio = 'ignore'; and cpOpts.detached = true; to the childProcess.spawn option object. see the relevant opn code

Testing on Ubuntu before the change:

  • With no browser open: starting react-scripts/start.js via yarn or npm then terminating the process via <Ctrl>c would also terminate the browser session

After:

  • With no browser open: starting react-scripts/start.js via yarn or npm then terminating the process via <Ctrl>c does not terminate the browser session.

This fixes #3113 for me on Ubuntu and causes no regressions on macOS Mojave (where I was unable to replicate the issue due to it not using xdg-open) Didn't get a chance to test on Windows so would be nice if someone else could give that a go.)

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 15, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 15, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@stale

This comment has been minimized.

Copy link

commented Dec 15, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 15, 2018

@iansu iansu removed the stale label Dec 15, 2018

@stale

This comment has been minimized.

Copy link

commented Jan 18, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added stale and removed stale labels Jan 18, 2019

@stale

This comment has been minimized.

Copy link

commented Feb 22, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 22, 2019

@evalexpr evalexpr force-pushed the evalexpr:dont-wait-for-app-to-exit branch from d330f36 to 420954f Feb 22, 2019

@stale stale bot removed stale labels Feb 22, 2019

@iansu iansu self-assigned this Feb 22, 2019

@mrmckeb

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

@W1lkins Out of interest, what happens if the browser is already open in Ubuntu (as it is now)? Does it still try to close the browser?

@evalexpr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@mrmckeb I've recorded some examples where I run a quick script:

#!/bin/bash -e

echo -e "Ubuntu version $(head -n2 /etc/os-release)\\n"
echo -e "Chrome version: $(google-chrome-unstable --version)\\n"
echo -e "Yarn version: $(yarn --version)\\n"
echo -e "Value of options: $(sed -n 99p test-app/node_modules/react-dev-utils/openBrowser.js)\\n"

(I now realise after recording I forgot to check create-react-app --version but it is the latest version: 2.1.8)

Pre-PR behaviour with browser open:
old-browser-open

Pre-PR behaviour with browser closed:
old-browser-closed

Post-PR behaviour with browser open:
new-browser-open

Post-PR behaviour with browser closed:
new-browser-closed

As you can see in the old behaviour (Pre-PR behaviour with browser closed), without wait: false the browser session ends when a SIGINT is sent to the react-scripts/start.js process. However, if the browser instance already exists and CRA isn't responsible for creating the initial session, it doesn't.

Hope this answers your query!

@iansu iansu closed this Mar 15, 2019

@iansu iansu reopened this Mar 15, 2019

@iansu

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Can we verify that this still works as expected on macOS and Windows? Also, can you rebase this branch with master so we can get the tests passing?

@iansu iansu added this to In progress in v3 via automation Mar 15, 2019

@iansu iansu added this to the 3.0 milestone Mar 15, 2019

@evalexpr evalexpr force-pushed the evalexpr:dont-wait-for-app-to-exit branch from 420954f to 4d2b9e6 Mar 15, 2019

@evalexpr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I've tested both starting from no browser open and starting with the browser already open on:

MacOS:

  • OS: Mojave 10.14
  • Yarn: 1.15.2
  • CRA: 2.1.8
  • Chrome: 74.0.3729.6 dev

Windows:

  • OS: 10 Home 64-Bit (10.0, Build 17134)
  • Yarn: 1.15.2
  • CRA: 2.1.8
  • Chrome: 72.0.3626.121

@iansu iansu merged commit 7864ba3 into facebook:master Mar 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details

v3 automation moved this from In progress to Done Mar 15, 2019

@iansu

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Thanks!

@evalexpr evalexpr deleted the evalexpr:dont-wait-for-app-to-exit branch Mar 15, 2019

@lock lock bot locked and limited conversation to collaborators Mar 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.