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

refactor: api-server's watch build process #11110

Merged
merged 7 commits into from
Aug 9, 2024

Conversation

callingmedic911
Copy link
Member

@callingmedic911 callingmedic911 commented Jul 28, 2024

Follow-up to #11109.

While working on #11109, I started making changes that turned into a larger refactor, that's why this separate PR:

  • Fix: Centralized debounce logic with precedence of flags (e.g., if a rebuild: false call happens while debounced, it takes precedence, even if subsequent debounced calls were with true).
  • Decoupled the build and HTTP server logic in watch.ts. The main motivation was to test parts of the build process I modified. This also improves readability with clearer responsibilities.
  • Added tests for the debounce with precedence logic.

@callingmedic911 callingmedic911 added release:fix This PR is a fix changesets-ok Override the changesets check labels Jul 28, 2024
@callingmedic911 callingmedic911 added this to the next-release-patch milestone Jul 28, 2024
@callingmedic911 callingmedic911 marked this pull request as draft July 28, 2024 02:14
@callingmedic911 callingmedic911 marked this pull request as ready for review July 28, 2024 03:52
Because we are not actually canceling if build has already started.
@callingmedic911 callingmedic911 self-assigned this Jul 28, 2024
Comment on lines 242 to 142
debouncedRebuild.cancel()
debouncedBuild()
await buildManager.run({ rebuild: false })
} else {
// If files have just changed, then rebuild
debouncedRebuild.cancel()
debouncedRebuild()
await buildManager.run({ rebuild: true })
Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM left a 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 Show resolved Hide resolved
const killApiServer = () => {
httpServerProcess?.emit('exit')
httpServerProcess?.kill()
serverManager.restartApiServer()
Copy link
Collaborator

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.

Copy link
Member Author

@callingmedic911 callingmedic911 Aug 8, 2024

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

@callingmedic911 callingmedic911 merged commit abdb184 into main Aug 9, 2024
46 checks passed
@callingmedic911 callingmedic911 deleted the aditya/refactor-watch-in-api-server branch August 9, 2024 02:51
dac09 added a commit to dac09/redwood that referenced this pull request Aug 9, 2024
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:fix This PR is a fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants