feat(nuxi): cli wrapper for self restart #18641
Conversation
|
|
antfu
changed the title
|
This is lovely 💯 |
You mean listening in https://github.com/nuxt/nuxt/blob/main/packages/nuxi/src/commands/dev.ts ? But the issue is that this file is already importing Nuxt dependencies where in the wrapper we don't have this caching issue |
That's also a huge performance gain to keep cache. What dependencies you are thinking would be cached and useful to be invalidated? |
|
I think performance isn't the issue here; a full restart would be an exceptional situation and most likely in order to flush the cache, for example a programmatic nuxt/module version upgrade + restarting of nuxt. |
|
The problem we are trying to solve here is the support for "Hard restart", where the cache is explicitly not desired (e.g. upgrade Nuxt itself). This shouldn't be a common operation as we already support "Soft restart" by config changes, etc. Performance is relevant here I think. |
|
Oh, and I forgot to reference, Vitest has been using this pattern for months to capture Node crashes and handle the restart: https://github.com/vitest-dev/vitest/blob/4fc492c61d6e01bc34bc807a6b57197a1914f823/packages/vitest/src/node/cli-wrapper.ts#L134-L135 It works quite nicely and we don't see issues with this approach. |
|
By the discussion with @danielroe, I just pushed a commit to extend the await nuxt.hooks.callHook('restart', { hard: true })Later, with the new CLI integration, we could keep the interface the same, making the transition invisible. |
packages/nuxi/src/constants.ts
Outdated
| * } | ||
| * ``` | ||
| */ | ||
| export const EXIT_CODE_RESTART = 44 |
There was a problem hiding this comment.
Although I assume this implementation is temporary, code 44 is reserved for UNIX (Channel number out of range). As we don't (and shouldn't) use code 0, i think we can simply use that code as an indicator that child was killed and expects to be restarted (otherwise use a higher number)
There was a problem hiding this comment.
Just to clarify, are you suggesting we use a different code than 44? 0 is the default, I think, and wouldn't indicate anything if we received it.
There was a problem hiding this comment.
I was checking this table: https://tldp.org/LDP/abs/html/exitcodes.html says 3-125 are free to use.
And yes, 44 was a number I picked randomly.
Go a bit deeper I found this:
https://www.cyberciti.biz/faq/linux-bash-exit-status-set-exit-statusin-bash/
It seems to me the codes are specially for bash to exit, I suppose it shouldn't have much impact on our side (+ the code is only be captured by our wrapper)
So anyway I changed it to 85 as it's defined in the previous link as Interrupted system call should be restarted
There was a problem hiding this comment.
We could use 1. Auto restarting with child unwanted exit codes too as well as signaling explicitly (also consistent with sighup used for restart)
There was a problem hiding this comment.
I am not sure if I get what you mean - are you suggesting we exit with process.exit(1)? Wouldn't that be indistinguishable with the normal error exit?
There was a problem hiding this comment.
A child normally doesn't exits with (1) but yeah it might introduce edge-cases.
Well, btw if we use fork, we can use the process communication channel to explicitly send a nuxt event (https://github.com/nuxt/cli/pull/22/files#diff-34d76a96faa930c4b9909c055beb422e601e2267f72ce12d6f15a1c2cc6ea914R57)
| const cliEntry = fileURLToPath(new URL('../dist/cli-run.mjs', import.meta.url)) | ||
|
|
||
| async function startSubprocess (preArgs: string[], postArgs: string[]) { | ||
| const child = await execa( |
There was a problem hiding this comment.
Perf: I guess we can use fork (or exec) from nod:child_process. This is also consistent with the final nuxi-ng implementation and provides a communication channel.
There was a problem hiding this comment.
I see we use execa in many places already, and the chunk is shared. I guess the performance won't be a big issue here.
I am just worried about using fork or exec might create more issues to handle on different OS, etc.
There was a problem hiding this comment.
Main usecase i added execa was search path compatibility which isn’t a case here. Also fork copies mem in unix then execs which by theory at least should be faster than loading node to the mem again.
There was a problem hiding this comment.
Could we mesure the performance difference here?
I like the simplicity of code and execa has some good features:
There was a problem hiding this comment.
Most of those benefits aren't relevant here and main benefit is using fork over exec with fine gained control. we are forking a known entry as process (not a on-off executation like other commands).
Finally this impl will be part of citty's dev server so this change is to make sure we are aligned :)
There was a problem hiding this comment.
Nice idea with introducing restart({ hard }) hook 👍🏼
Other than mentioned comments above, i think we can move forward with it unblocking features like #19394 (also adding hook types in next release!)
As I have drafted in nuxt/cli#22, the final new implementation of hard start can be completely different with the CLI server being alive across reloads. This means hard restart will have behavior differences and it might be internally aware of it.
|
I think we can go ahead and merge this and iterate on the implementation if needed. (It's not clear to me how much refactoring this is required given that we will shortly be migrating to nuxi-ng, but I'm not meaning to shut down discussion.) |
|
Please next times, wait on comments to be resolved before moving forward! |
🔗 Linked issue
❓ Type of change
📚 Description
This would allow Nuxt to do hard restart, for self upgrading or other stuffs.
📝 Checklist