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

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

Merged
merged 3 commits into from Feb 8, 2019

Conversation

Projects
None yet
3 participants
@pimlie
Copy link
Contributor

pimlie commented Feb 7, 2019

fix: remove slash from warning text
fix: dont force-exit when explicitly disabled
chore: add force-exit tests for command

Follow-up of: #4958

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

As discussed on discord, some fixes and stability improvements for my pr from yesterday

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 7, 2019

fix: dont force-exit when explicitly disabled
chore: add tests for force-exit behaviour
Show resolved Hide resolved packages/cli/src/command.js Outdated
@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 7, 2019

As there appear to be some confusion (not in the least by myself), here is to clarify the force-exit behaviour:

  • We want nuxt-cli to be able to forcibly exit after NuxtCommand.run() has finished. This behaviour can be toggled on the cli by using --force-exit and is independent of the actual command that is run (its an abstract feature).
    That means you can also run nuxt start --force-exit and then nuxt will immediately exit after it started listening. Maybe not really usefull normally but it could be used to quickly or automatically test whether the Nuxt server wa setup correctly and didnt threw any errors on server middleware etc.

  • If you use --force-exit then process.exit will always be called with exitCode 0. This is because this way you have a way to ignore bad code and force eg your ci pipeline to continue. Besides, when there are fatal errors then nuxt will already exit with a non-zero exit code in the consola reporter so we dont have to check that here (but we might want to anyway in the future for non-fatal errors).

  • For some commands we want an additional check, nuxt build and nuxt generate are commands of which we know they should immediately exit after NuxtCommand.run() has finished. For this we added a (reversed) additional check on this.isServer. isServer means we assume the command is starting a server and its normal the command keeps running after NuxtCommand.run() finishes. So the reverse is that when !isServer it shouldnt keep running so we only force-exit when bin/nuxt-cli.js doesnt exit by itself.

  • For this additional check we dont always want to call process.exit as it would hide bad code. So instead of calling process.exit immediately we set an unref timeout that calls process.exit. By unreferencing the timeout node will just normally exit when the event loop is empty, but if there are unreleased listeners still using the event loop the timeout will trigger after 5 seconds and still exit nuxt.

-- edit --

  • Forgot one thing to mention: We also want to be developer friendly by having the possibility to skip that additional check. Not sure when that would be useful but still I think we should provide the possibility, so thats what the isExplicitArgument method is for. When you use nuxt generate --no-force-exit then the timeout will never be set (and nuxt will still hang if you have badly written code).

-- /edit--

The difference between nuxt v2 and v3 will be that the additional check for nuxt build/generate is a warning and exits with code 0 in v2 while in v3 it will be a (fatal?) error and will exit with non-zero code.

Show resolved Hide resolved packages/cli/src/command.js Outdated
@pi0

This comment has been minimized.

Copy link
Member

pi0 commented Feb 7, 2019

  • That means you can also run nuxt start --force-exit and then nuxt will immediately exit after it started listening. Maybe not really usefull normally but it could be used to quickly or automatically test whether the Nuxt server wa setup correctly and didnt threw any errors on server middleware etc.

This throws inappreciate warning/error message.

  • For some commands we want an additional check, nuxt build and nuxt generate are commands of which we know they should immediately exit after NuxtCommand.run() has finished. For this we added a (reversed) additional check on this.isServer

Two remarks:

  • isServer check is not accurate. We rely on an implicit option to determine it.
  • I think it could be better to explicitly enable force-exit by default in their cli options. And probably allow --no-force-exit as well.
@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 7, 2019

This throws inappreciate warning/error message.

I dont think it does? As mentioned above when you explicitly set --force-exit I dont set the timeout and you dont get the warning message. See here:

  • isServer check is not accurate. We rely on an implicit option to determine it.
  • I think it could be better to explicitly enable force-exit by default in their cli options. And probably allow --no-force-exit as well.

By using isServer I wanted to differentiate between the two options, because using --force-exit should be always call process.exit when cmd.run finishes and the additional check is only force exit when nuxt-cli doesnt exit by itself. It doesnt feel correct to me to use --force-exit for both situations.

Maybe we should otherwise add a cmd property?

export default {
name: 'dev',
isServer: true,
run() ...
}
@pi0

This comment has been minimized.

Copy link
Member

pi0 commented Feb 7, 2019

@pimlie Exactly. I suggest allowing setting forceExit in export default of build and generate command that explicitly needs them. Less config and best behavior by default. User can disable them if rare conditions by --no-force-exit them. And no more relies on isServer.

@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 7, 2019

I added the possibility of making the default option value a fn. Good or not? Description is now a bit less descriptive because its the same description for both --force-exit as --no-force-exit.

@pimlie

This comment has been minimized.

Copy link
Contributor Author

pimlie commented Feb 8, 2019

Another day and another night thinking about this in bed. Or should we just leave the default option value and its description as is/was and do this:

const cmdShouldFinish = ['build', 'generate'].includes(this.cmd.name)
const forceExitByUser = this.isUserSuppliedArg('force-exit')
if (this.argv['force-exit'] || (cmdShouldFinish && !forceExitByUser)) {
  runResolve.then(() => forceExit(this.cmd.name, forceExitByUser ? false : forceExitTimeout))
}

So that would actually be still some sort of a isServer variable but instead of looking at options.hostname it just checks the cmd.name. I like this better I think

@pi0

pi0 approved these changes Feb 8, 2019

@pi0 pi0 merged commit 3e9eee2 into nuxt:dev Feb 8, 2019

9 of 10 checks passed

ci/circleci: audit Your tests failed on CircleCI
Details
Semantic Pull Request ready to be squashed
Details
[ci.azure] nuxt.js #20190207.13 succeeded
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

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

fix: dont force exit when it was explicitly disabled (#4973)
* fix: remove slash from warning text

* fix: dont force-exit when explicitly disabled

chore: add tests for force-exit behaviour

* feat: default option value can be fn

@pi0 pi0 referenced this pull request Feb 25, 2019

Closed

chore(release): v2.4.4 #5109

@pimlie pimlie deleted the pimlie:fix-forced-exit branch Mar 4, 2019

@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.