You no longer need to press ctrl-c to exit if you say no to a new port #1887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lfdebrux
requested review from
joelanman and
BenSurgisonGDS
and removed request for
joelanman
lfdebrux
force-pushed
the
ldeb-refactor-find-available-port
branch
2 times, most recently
from
82ad5ff
to
c469384
Compare
lfdebrux
changed the title
lfdebrux
force-pushed
the
ldeb-refactor-find-available-port
branch
from
c469384
to
1cfde8a
Compare
|
There is one downside to this approach; changing the port number in |
lfdebrux
force-pushed
the
ldeb-refactor-find-available-port
branch
2 times, most recently
from
31ffde8
to
e3d4354
Compare
BenSurgisonGDS
approved these changes
Jan 4, 2023
|
Thanks @BenSurgisonGDS! I've decided to split those changes into a separate PR, #1890, would you mind approving that? |
|
this sounds very cool! Reviewing now... |
lfdebrux
force-pushed
the
ldeb-refactor-find-available-port
branch
3 times, most recently
from
8c12b17
to
b6cf313
Compare
joelanman
reviewed
Jan 5, 2023
| @@ -48,7 +47,8 @@ function generateAssetsSync ({ verbose } = {}) { | |||
|
|
|||
| function clean () { | |||
| // doesn't clean extensions.scss, should it? | |||
| del.sync(['public/**', '.port.tmp', portTmpFile]) | |||
| // TODO: These files are deprecated, remove this line in v14? | |||
| del.sync(['public/**', '.port.tmp', '.tmp/port.tmp']) | |||
There was a problem hiding this comment.
why do we still need to delete the port files?
There was a problem hiding this comment.
Just in case the files are still around for some reason. It's not strictly necessary, just being neat.
lfdebrux
force-pushed
the
ldeb-refactor-find-available-port
branch
from
b6cf313
to
03f8989
Compare
joelanman
approved these changes
Jan 5, 2023
There was a problem hiding this comment.
looks good thanks! Not sure if we want to merge for the next release or not @nataliecarey ?
Argument `app` is never used.
This fixes #462, as the `process.exit(0)` is no longer done within the `nodemon` process. It also means we're not running portScanner every time the kit restarts, and we can remove the code saving the port number to a file. Note changing the port number in `app/config.json` will now require a manual restart.
BenSurgisonGDS
force-pushed
the
ldeb-refactor-find-available-port
branch
from
03f8989
to
e3c47cd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the code around finding an available port. We now run portScanner only once, when the dev server is started.
This fixes bug #462 (doesn't exit if you say no to a new port), as the prompt to the user and
process.exit(0)take place outside ofnodemon. This change also means we can remove the code saving the port number to a file.