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

Conversation

@davidkwan95
Copy link
Contributor

davidkwan95 commented Jan 11, 2018

This PR should fix issue #2716

@codecov

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

ndelangen commented Jan 11, 2018

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

@Hypnosphi

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidkwan95 commented Jan 11, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidkwan95 commented Jan 11, 2018

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

@ndelangen

This comment has been minimized.

Copy link
Member

ndelangen commented Jan 20, 2018

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 Jan 20, 2018
@ndelangen ndelangen merged commit 34b98a8 into storybookjs:master Jan 20, 2018
19 of 20 checks passed
19 of 20 checks passed
codecov/patch 0% of diff hit (target 35.51%)
Details
Better Code Hub ✅ Better Code Hub approves this code
Details
CodeFactor 1 issue fixed.
Details
Node Security No known vulnerabilities found
Details
bitHound - Code No failing files.
Details
bitHound - Dependencies No failing dependencies.
Details
ci/chromatic 71 specs unchanged.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cli Your tests passed on CircleCI!
Details
ci/circleci: cli-latest-cra Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: example-kitchen-sinks Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: react-native Your tests passed on CircleCI!
Details
ci/circleci: storybook Your tests passed on CircleCI!
Details
ci/circleci: unit-test Your tests passed on CircleCI!
Details
codebeat no reportable quality changes
Details
codecov/project 35.51% remains the same compared to 402183b
Details
deploy/netlify Deploy preview ready!
Details
security/snyk No new issues
Details
@davidkwan95 davidkwan95 deleted the davidkwan95:patch-1 branch Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.