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

Do not check for interactive session to shut down dev server #8845

Merged
merged 1 commit into from
May 4, 2020

Conversation

jeremywadsack
Copy link
Contributor

@jeremywadsack jeremywadsack commented Apr 16, 2020

Fixes #8688

As discussed in #8868, Docker sessions shut down the dev server before it gets a chance to use it because Docker appears to be "interactive" and then closes stdin immediately. The workarounds were to set CI=true (which worked in some cases but not others) or to configure the Docker container to maintain an open TTY.

This shouldn't be necessary and is affecting both CI deployments and other environments.

The problem is a change in #7203, which intended to close the development server when stdin closed because otherwise it was left hanging open. That now is closing prematurely in cases where it's used in Docker.

During development of #7203, the check for an interactive session was added to fix it crashing in CRA's CI. However that didn't resolve the issue for some uses of Phoenix development watcher, which was fixed by checking that the CI environment variable is not set. The latter check should resolve the issue for CRA CI by itself — most CI systems set that variable.

While I couldn't figure out how to test a branch as an installed module (because of the monorepo nature), I did test this by hacking my Circle CI configuration to apply the patch and verified that resolved the CI issue for me:

sed -i '/process.env.CI/ s/isInteractive [|]*//' node_modules/react-scripts/scripts/start.js

I'm opening this PR in the hopes that it runs the CRA CI and will verify if this does not introduce a CI crash.

…interactive even when not started with a tty
@facebook-github-bot
Copy link

Hi @jeremywadsack!

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 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 to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

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

@endophage
Copy link

LGTM!

Not a maintainer but straight-foward default behaviour with a simple config toggle is better than some magic auto-detection any day.

@ianschmitz ianschmitz added this to the 4.0 milestone May 2, 2020
@ianschmitz ianschmitz merged commit 2975939 into facebook:master May 4, 2020
@jeremywadsack jeremywadsack deleted the stdin-tty branch May 5, 2020 19:08
@luispabon
Copy link

Is there any chance this fix can be released as 3.4.2? Looks like a regression in 3.4.1

@pluma
Copy link

pluma commented May 15, 2020

This has been merged for over a week and fixes a regression from 3.4.0 to 3.4.1. I don't want to sound impatient but could we please get a hotfix?

@lock lock bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[React-Scripts] v3.4.1 fails to start in Docker
8 participants