-
Notifications
You must be signed in to change notification settings - Fork 1k
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
refactor: api-server's watch
build process
#11110
Conversation
Because we are not actually canceling if build has already started.
packages/api-server/src/watch.ts
Outdated
debouncedRebuild.cancel() | ||
debouncedBuild() | ||
await buildManager.run({ rebuild: false }) | ||
} else { | ||
// If files have just changed, then rebuild | ||
debouncedRebuild.cancel() | ||
debouncedRebuild() | ||
await buildManager.run({ rebuild: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: do we want to await
? we weren't before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sure, I think I'd await it. I feel like maybe if we don't await this here then you might end up having to keep track to ensure one build function is finished before triggering another one? (In the case where a build takes longer than the debounce itself.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I personally find it much easier to follow.
Tested locally as a sanity check and everything seems good.
packages/api-server/src/watch.ts
Outdated
const killApiServer = () => { | ||
httpServerProcess?.emit('exit') | ||
httpServerProcess?.kill() | ||
serverManager.restartApiServer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okay not awaiting this async function?
I wonder if maybe that could contribute to #10876 but that's speculation and addressing that issue is not truly within the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this hypothesis but with large enough project, it's possible that this unwaited concurrent restartApiServer
could try to kill and start api server at the same time. await
ing makes sense as it prioritizes correctness over perf.
sidenote: I got distracted trying to setup no-floating-promises but configuration wasn't as straighforward and with all the hoops eslint process crashed with some OOM. I gave up there.
…pload-link * 'main' of github.com:redwoodjs/redwood: refactor: api-server's `watch` build process (redwoodjs#11110) fix: Comment out unused import line in seed script template (redwoodjs#11171)
Follow-up to #11109.
While working on #11109, I started making changes that turned into a larger refactor, that's why this separate PR:
rebuild: false
call happens while debounced, it takes precedence, even if subsequent debounced calls were withtrue
).watch.ts
. The main motivation was to test parts of the build process I modified. This also improves readability with clearer responsibilities.