Remove nodemon from dev server lifecycle - #4407
Conversation
@wasp.sh/spec
@wasp.sh/wasp-cli
@wasp.sh/wasp-cli-darwin-arm64-unknown
@wasp.sh/wasp-cli-darwin-x64-unknown
@wasp.sh/wasp-cli-linux-arm64-glibc
@wasp.sh/wasp-cli-linux-x64-glibc
@wasp.sh/wasp-cli-linux-x64-musl
commit: |
Deploying wasp-docs-on-main with
|
| Latest commit: |
ef380f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5648703.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://miho-native-server-process-w.wasp-docs-on-main.pages.dev |
| import qualified Wasp.Job as J | ||
| import Wasp.Job.Except (ExceptJob, toExceptJob) | ||
| import Wasp.Job.Process (runNodeCommandAsJob, runNodeCommandAsJobWithExtraEnv) | ||
| import Wasp.Job.Node (runNodeCommandAsJob, runNodeCommandAsJobWithExtraEnv) |
There was a problem hiding this comment.
There were some Node.js specific bits that got mixed with generic process handling, so I separated them.
| #if !mingw32_HOST_OS | ||
| import qualified System.Posix.Signals as Signals | ||
| #endif | ||
|
|
||
| withGracefulTermination :: IO a -> IO a | ||
| #if mingw32_HOST_OS | ||
| withGracefulTermination = id | ||
| #else | ||
| withGracefulTermination action = do | ||
| targetThreadId <- myThreadId | ||
| let interruptCurrentCommand = Signals.Catch $ throwTo targetThreadId UserInterrupt | ||
| bracket | ||
| (mapM (installHandler interruptCurrentCommand) [Signals.sigINT, Signals.sigTERM]) | ||
| restoreHandlers | ||
| (const action) | ||
| where | ||
| installHandler handler signal = do | ||
| previousHandler <- Signals.installHandler signal handler Nothing | ||
| return (signal, previousHandler) | ||
|
|
||
| restoreHandlers = | ||
| mapM_ $ \(signal, previousHandler) -> | ||
| Signals.installHandler signal previousHandler Nothing | ||
| #endif |
There was a problem hiding this comment.
This is the wildest bit of the implementation - to enable cleanup after wasp start is interrupted.
There was a problem hiding this comment.
Why is bracket not enough?
Also, I am adding Resourcet to Command in #4504, might that be useful?
| @@ -0,0 +1,29 @@ | |||
| module Wasp.Util.Glob | |||
There was a problem hiding this comment.
Extract from waspignore module. Shared between waspignore and server path watching.
| -- Also makes sure to remove any redundant file drafts that have been left on the disk from before. | ||
| -- It is smart when writing, so it doesn't write file drafts that are already written on the disk from before. | ||
| synchronizeFileDraftsWithDisk :: Path' Abs (Dir GeneratedAppDir) -> [FileDraft] -> IO () | ||
| synchronizeFileDraftsWithDisk :: Path' Abs (Dir GeneratedAppDir) -> [FileDraft] -> IO [GeneratedAppPathChange] |
There was a problem hiding this comment.
After sync file drafts, we return a list of written and deleted files. The idea is to be able to detect which generated files changes happened so we can react to it e.g. restart the server process.
8a731a0 to
9e7d550
Compare
82296d7 to
5bd61ac
Compare
5bd61ac to
063ac16
Compare
063ac16 to
12e2fca
Compare
cprecioso
left a comment
There was a problem hiding this comment.
Random comments, didn't do proper review yet
| ### 🐞 Bug fixes | ||
|
|
||
| - Fixed a race condition where `useAuth()` could return stale user data after an action that modifies the `User` entity (by @okxint). ([#4343](https://github.com/wasp-lang/wasp/issues/4343)) | ||
| - `wasp start` now manages the development server process directly instead of relying on the generated server's `nodemon`, improving restart behavior. ([#4407](https://github.com/wasp-lang/wasp/pull/4407)) |
There was a problem hiding this comment.
nitpick
| - `wasp start` now manages the development server process directly instead of relying on the generated server's `nodemon`, improving restart behavior. ([#4407](https://github.com/wasp-lang/wasp/pull/4407)) | |
| - `wasp start` now manages the development server process directly instead with `nodemon`, improving stability. ([#4407](https://github.com/wasp-lang/wasp/pull/4407)) |
| @@ -1 +1 @@ | |||
| `npm start` to run server in development mode (it reloads on changes). | |||
| `npm start` runs the already bundled server in development mode. Use `wasp start` for automatic rebuilds and restarts while developing. | |||
There was a problem hiding this comment.
I'd remove the wasp start note straight away. Users either see this in production or don't see it in development.
I might even remove the README.
| devServerStartExecutable = "node" | ||
|
|
||
| devServerStartArgs :: [String] | ||
| devServerStartArgs = ["--enable-source-maps", "-r", "dotenv/config", "bundle/server.js"] |
There was a problem hiding this comment.
prolly not for now but: https://nodejs.org/docs/latest-v24.x/api/environment_variables.html#cli-options
| devServerStartExecutable :: String | ||
| devServerStartExecutable = "node" |
There was a problem hiding this comment.
nitpick i'd make the command + args just a single array, we can head it later. or a tuple at least. it doesn't make sense to define one thing without the other IMO
| devServerStartCommand :: String | ||
| devServerStartCommand = unwords $ devServerStartExecutable : devServerStartArgs |
There was a problem hiding this comment.
and we could just unwords this in the callsite, we "don't know" here yet where this will be used and each callsite is in charge of formatting multiple arguments and escaping spaces etc.
There was a problem hiding this comment.
not reviewing this yet, maybe the review on #4500 makes it easier
| #if mingw32_HOST_OS | ||
| stopProcessTree processHandle rootExitAsync _ = do | ||
| void (try (P.terminateProcess processHandle) :: IO (Either SomeException ())) | ||
| waitForAsync rootExitAsync hardStopTimeoutMicroseconds | ||
| #else |
There was a problem hiding this comment.
I'd extract the system-specific parts into their own module (e.g. Job.Process.LongRunning.System), so the templated functions are more scoped to a single place instead of mixed with "regular" code
| recursiveFileGlobsWithExtensions :: FilePath -> [String] -> [String] | ||
| recursiveFileGlobsWithExtensions dir extensions = | ||
| (\ext -> dir FP.</> "**" FP.</> "*" ++ ext) <$> extensions | ||
|
|
||
| dirAndDescendantsGlobs :: FilePath -> [String] | ||
| dirAndDescendantsGlobs dir = | ||
| [ dir, | ||
| dir FP.</> "**" FP.</> "*" | ||
| ] |
There was a problem hiding this comment.
Patterns are Monoids, so we can use its type directly and <> instead of FilePaths, which is not semantically accurate. Plus the semantic helpers at System.FilePath.Glob.Primitive. We can use / directly since it's the only valid separator for globs, independently of the system:
| recursiveFileGlobsWithExtensions :: FilePath -> [String] -> [String] | |
| recursiveFileGlobsWithExtensions dir extensions = | |
| (\ext -> dir FP.</> "**" FP.</> "*" ++ ext) <$> extensions | |
| dirAndDescendantsGlobs :: FilePath -> [String] | |
| dirAndDescendantsGlobs dir = | |
| [ dir, | |
| dir FP.</> "**" FP.</> "*" | |
| ] | |
| recursiveFileGlobsWithExtensions :: Pattern -> [String] -> [Patterns] | |
| recursiveFileGlobsWithExtensions dir extensions = | |
| ((dir <> "/**/*" <>) . literal) <$> extensions | |
| dirAndDescendantsGlobs :: Pattern -> [Pattern] | |
| dirAndDescendantsGlobs dir = | |
| [ dir, | |
| dir <> "/**/*" | |
| ] |
(i tried it in GHCi and it all works as expected)
| Path' Abs (Dir Generator.GeneratedAppDir) -> | ||
| CompileOptions -> | ||
| IO ([CompileWarning], [CompileError]) | ||
| IO CompileResult |
There was a problem hiding this comment.
Seems like what we actually want is a
type Compilation = ExceptT CompileError (WriterT [CompileWarning] IO) [FileOrDirPathRelativeTo Generator.GeneratedAppDir]
Description
Remove
nodemonfrom the generated development server.wasp startnow owns the server process, decides when it needs updating, and cleans it up itself.This fixes stale server processes after a failed compile or bundle. It also avoids restarting a healthy server when a recompile did not change anything the server uses.
Closes #1844.
Server effects
Every successful recompile produces one
ServerEffect:NoServerEffectRestartServer.envchanges.RebundleAndRestartServerWhen several files change together, the strongest required effect wins.
The effect is based on both the saved project files and the generated files whose checksums actually changed. A
main.wasp.tssave alone does not restart the server. If that edit changes generatedserver/src,server/package.json, orserver/.env, it does.Trade-off: generated SDK paths are ignored. We assume every server-relevant SDK change also changes generated server source or a project
srcruntime input.Server process controller
The controller owns the current server process and handles compile results one at a time:
wasp startrunning.wasp startrunning.If the server exits by itself, the controller cleans up any surviving descendants and waits for the next successful compile.
Managed subprocesses
Wasp.Job.Subprocess.spawnstarts a child process whose lifetime is tied to its Job. Releasing it stops the whole owned process tree, drains or closes output, and waits for cleanup before returning.The platform-specific process-tree work lives in
Wasp.Job.Subprocess.System: process groups and signals on Unix, process jobs on Windows, and Linux/procchecks for zombie-only groups.wasp startalso installs graceful SIGINT/SIGTERM handling so process cleanup finishes before Wasp exits.Review map
Just code moving:
Test.Process.Util; their behavior is unchanged.Generated or mechanical changes:
nodemondependency.nodemon.jsonand changing the generated server package scripts.Not just a rename:
Jobresources.Type of change
Checklist
I tested my change in a Wasp app to verify that it works as intended.
🧪 Tests and apps:
examples/kitchen-sink/e2e-tests.waspc/data/Cli/templates, as needed.examples/, as needed.examples/tutorials) I updated the tutorial in the docs (and vice versa).📜 Documentation:
web/docs/.🆕 Changelog: (if change is more than just code/docs improvement)
waspc/ChangeLog.mdwith a user-friendly description of the change.web/docs/migration-guides/.versioninwaspc/waspc.cabalto reflect the changes I introduced.