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

feat: show warning on forced exit #4958

Merged
merged 9 commits into from Feb 6, 2019

Conversation

Projects
None yet
5 participants
@pimlie
Copy link
Contributor

pimlie commented Feb 5, 2019

feat: warn when nuxt doesnt exit automatically
fix: add default value when no charsPerLine specified to foldLines fn
feat: make maxCharsPerLine a fn as the console width can be changed while running a command
fix: move format settings to separate module so we can mock them
feat: add different box types
fix: move forceExitTimeout to settings

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

In previous versions Nuxt always called process.exit when eg the build command was finished. Fortunately that has been removed as calling process.exit hides badly written code. But to improve DX I think its better to (try to) detect when Nuxt doesnt exit automatically and show a warning when that happens. This resolves #4944

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

pimlie added some commits Feb 5, 2019

feat: warn when nuxt doesnt exit automatically
fix: add default value when no charsPerLine specified to foldLines fn

feat: make maxCharsPerLine a fn as the console width can be changed while running a command

fix: move format settings to separate module so we can mock them

feat: add different box types
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #4958 into dev will decrease coverage by 0.51%.
The diff coverage is 24.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #4958      +/-   ##
==========================================
- Coverage   92.78%   92.27%   -0.52%     
==========================================
  Files          73       74       +1     
  Lines        2426     2447      +21     
  Branches      596      603       +7     
==========================================
+ Hits         2251     2258       +7     
- Misses        160      169       +9     
- Partials       15       20       +5
Impacted Files Coverage Δ
packages/cli/src/options/common.js 50% <ø> (ø) ⬆️
packages/cli/src/list.js 77.77% <0%> (-4.58%) ⬇️
packages/cli/src/utils/index.js 52.11% <0%> (-5.7%) ⬇️
packages/cli/src/setup.js 70% <0%> (ø) ⬆️
packages/cli/src/utils/constants.js 100% <100%> (ø)
packages/cli/src/utils/formatting.js 60% <12.5%> (-16.2%) ⬇️
packages/cli/src/command.js 91.46% <62.5%> (-0.75%) ⬇️

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 4e30bda...8029070. Read the comment docs.

@manniL manniL requested a review from nuxt/core-team Feb 5, 2019

Show resolved Hide resolved packages/cli/src/command.js Outdated
Show resolved Hide resolved packages/cli/src/command.js Outdated
Show resolved Hide resolved packages/cli/src/utils/settings.js Outdated
@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 6, 2019

@pi0 What do you think about adding a --force-exit cli option which is default true for now and default false as of v3. Similar to https://jestjs.io/docs/en/cli#forceexit ?

@pi0

This comment has been minimized.

Copy link
Member

pi0 commented Feb 6, 2019

Good idea @pimlie

feat: add --force-exit cli option
more changes after review
@manniL

This comment has been minimized.

Copy link
Member

manniL commented Feb 6, 2019

Hmm. Shouldn't we classify that PR as a fix? 🤔

@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 6, 2019

FYI this is a WIP, still future-proofing the force-exit option. Will update when ready Ready for review

@manniL manniL added the WIP label Feb 6, 2019

}, timeout * 1000)
exitTimeout.unref()
} else {
process.exit(0)

This comment has been minimized.

Copy link
@pimlie

pimlie Feb 6, 2019

Author Contributor

When force-exit is explicitly used (in v3) this should be 0, unless we can let the command's run method returns a status.


// TODO: Change this to a fatal error in v3
process.stderr.write(warningBox(msg))
process.exit(0)

This comment has been minimized.

Copy link
@pimlie

pimlie Feb 6, 2019

Author Contributor

Maybe use https://github.com/cowboy/node-exit instead? (jest does but the issue that package links to is already fixed)

Show resolved Hide resolved packages/cli/src/utils/index.js Outdated

pimlie added some commits Feb 6, 2019

fix: use exit instead of process.exit
use string template

fix: cli test should use mock
@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 6, 2019

ready again

@manniL manniL removed the WIP label Feb 6, 2019

Show resolved Hide resolved packages/cli/src/setup.js Outdated

Atinux and others added some commits Feb 6, 2019

@Atinux Atinux self-requested a review Feb 6, 2019

@Atinux

Atinux approved these changes Feb 6, 2019

Copy link
Member

Atinux left a comment

This is an awesome PR @pimlie 👏

@pi0

pi0 approved these changes Feb 6, 2019

@pi0 pi0 merged commit 5094d9c into nuxt:dev Feb 6, 2019

10 checks passed

Semantic Pull Request ready to be squashed
Details
[ci.azure] nuxt.js #20190206.14 succeeded
Details
ci/circleci: audit Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint-app Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-types Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details

@pimlie pimlie deleted the pimlie:feat-warn-forced-exit branch Feb 7, 2019

@pimlie pimlie referenced this pull request Feb 7, 2019

Merged

fix: dont force exit when it was explicitly disabled #4973

1 of 7 tasks complete

clarkdo added a commit that referenced this pull request Feb 19, 2019

@pi0 pi0 referenced this pull request Feb 25, 2019

Closed

chore(release): v2.4.4 #5109

@pi0 pi0 referenced this pull request Mar 14, 2019

Merged

v2.5.0 #5237

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