-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Closes webpack dev server and exits process on "end" stdin #7203
Conversation
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 [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I've rerun the tests a couple of times and they keep failing on the |
My team is interested in seeing these changes merged as well. Does anyone have advice concerning how we might contribute to getting the tests to pass? According to the CI logs it looks like no tests are failing, but rather something is stalling during the runs. |
@kelseyleftwich, @aj-foster - I've fixed CI but i want to confirm that the change doesn't break it for your use case. Can you test it for me with the updated logic? See e44d202. |
Unfortunately, I haven't been able to make it work as a Phoenix development watcher. In that context, Wish I could come here with a solution rather than just bad news, but what's happening in CI doesn't make sense to me. |
I have the same use case as @aj-foster, so I would like to propose a solution: const exitOnStdinEnd = process.env.EXIT_ON_STDIN_END === 'true';
if (isInteractive || exitOnStdinEnd) {
// Gracefully exit when stdin ends
process.stdin.on('end', function() {
devServer.close();
process.exit();
});
process.stdin.resume();
} I know it's not ideal, but it would work for us. @ianschmitz What do you think? |
Hmm not super pumped on adding another env variable. I'm sure there's another way to accomplish this? |
@ianschmitz What if we use the CI env variable instead of if (process.env.CI !== 'true') {
// Gracefully exit when stdin ends
process.stdin.on('end', function() {
devServer.close();
process.exit();
});
process.stdin.resume();
} |
Looks like a good solution to me! Since this doesn't add more env variables. |
How about |
@ianschmitz yes, that would work for us! |
This is the same functionality as PR #3430 originally submitted by @jakehasler
The original PR broke the CI. It went unaddressed, went stale, and was automatically closed.
Like Jake, my team is integrating CRA with an elixir-phoenix app and the CRA isn't getting closed when the main process is killed.