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

using process.exitCode instead of process.exit() #2717

Merged
merged 2 commits into from
Jan 20, 2018
Merged

using process.exitCode instead of process.exit() #2717

merged 2 commits into from
Jan 20, 2018

Conversation

davidkwan95
Copy link
Contributor

This PR should fix issue #2716

@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

Merging #2717 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2717   +/-   ##
=======================================
  Coverage   35.51%   35.51%           
=======================================
  Files         434      434           
  Lines        9501     9501           
  Branches      986     1009   +23     
=======================================
  Hits         3374     3374           
+ Misses       5467     5459    -8     
- Partials      660      668    +8
Impacted Files Coverage Δ
app/react/src/server/build.js 0% <0%> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/react/src/server/babel_config.js 0% <0%> (-76.67%) ⬇️
app/polymer/src/server/utils.js 0% <0%> (-53.58%) ⬇️
.../ui/src/modules/ui/components/addon_panel/index.js 34.78% <0%> (ø) ⬆️
app/react-native/src/preview/story_kind.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Info.js 0% <0%> (ø) ⬆️
...modules/ui/components/stories_panel/text_filter.js 30.98% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/shortcuts_help.js 12.85% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/layout.js 12.5% <0%> (ø) ⬆️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 402183b...a4f2768. Read the comment docs.

@ndelangen
Copy link
Member

Thank you for this PR, Looks good. Can you teach my why the issue occurred and why this fixes it?

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 11, 2018

I'm intrigued as well. Docs actually say that using process.exit() is not a recommended practice, but the reason is that it may force the exit too early which is contrary to the referenced issue

@davidkwan95
Copy link
Contributor Author

I'm not too sure, but I noticed that calling process.exit() inside the callback function seems to wait for some process to be finished before exitting. I'm assuming it has something to do with webpack or node.
Without calling the process.exit(), we will be able to exit the program after the event loop is done, but since we got an error, we need to set the exit code to 1 by using process.exitCode = 1

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 11, 2018

after the event loop is done

If there's some async process running, event loop won't be done until the process is finished. Did you check if your fix is working?

@davidkwan95
Copy link
Contributor Author

I've checked that the fix works in my current project.

@ndelangen
Copy link
Member

So I've actually hit this problem now on an other library.
danielstjules/jsinspect#69

I'm ready to approve this.

@ndelangen ndelangen self-assigned this Jan 20, 2018
@ndelangen ndelangen self-requested a review January 20, 2018 09:01
@ndelangen ndelangen merged commit 34b98a8 into storybookjs:master Jan 20, 2018
@davidkwan95 davidkwan95 deleted the patch-1 branch January 21, 2018 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants