-
Notifications
You must be signed in to change notification settings - Fork 378
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
Fix hanging assets watchers #1922
base: master
Are you sure you want to change the base?
Conversation
fa1cdfe
to
36df275
Compare
Repro project: https://github.com/jleverenz/nestjs-assets-hang-repro |
actions/build.action.ts
Outdated
|
||
if (!watchMode) { | ||
this.assetsManager.closeWatchers(); | ||
} | ||
|
||
return; |
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 sure that this is going to work as expected? Regardless of whether it worked before, closing watchers immediately is a breaking change (given that previously we waited till the compilation/build has completed)
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 went through a fair number of manual test cases I came up with. I didn't see any automated test coverage for this though. I've been thinking a bit how this might be done, but haven't come up with anything yet.
I think this matches the original intent, but it would be good if someone else could help test / validate.
I'm new to the code base, but from what I can tell, this change would bring it in line with behavior when using tsc. However, it is still a change in behavior for webpack, and possibly not even the desired behavior?
Here are the test cases I tried, and the observed behavior.
Command used: npx nest start [--watch]
- With `tsc` & no "watchAssets"
- without `--watch`
- starts up chokidar filewatcher
- starts file copy before compilation (async without await)
- assets copied at start-up only, assets not updated on watch
- application exits
- with `--watch`
- starts up chokidar filewatcher
- starts file copy before compilation (async without await)
- assets copied at start-up only, assets *not* updated on watch
- this includes after code changes that trigger a re-compile
- application does not exit, in watch mode
- With `tsc` & top-level "watchAssets"
- without `--watch`
- same behavior as above
- with `--watch`
- starts up chokidar filewatcher
- starts file copy before compilation (async without await)
- assets copied at start-up, assets *are* updated on watch
- application does not exit, in watch mode
- With `webpack` & no "watchAssets"
- without `--watch`
- same behavior as `tsc` case
- **bug** does not exit after application exits, due to open chokidar watchers
- with `--watch`
- same behavior as `tsc` case
- With `webpack` & top-level "watchAssets"
- without `--watch`
- same behavior as above, including the **bug**
- with `--watch`
- same behavior as `tsc` case
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.
Did a little more experimenting. It looks like maybe compiler.run
is blocking whereas webpackCompiler.run
is not? If that's the case, maybe a different approach is needed to get the intended behavior..
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.
Two implementation ideas:
- Convert
compiler.run
function to async for any compilers and await them. This would standardize compiler wrapper interface. In the case oftsc
, it actually blocks, but in the case of webpack, it can be awaited. - Keep the different interfaces, but centralize the asset manager code in the builder action.
In both cases, the desired behavior would be:
- always exit a non-listening app when not in watch mode (e.g. standalone apps) (the original bug / breaking change)
- close asset file watchers after compilation completes in non-watch modes
- currently this just kicks off the polling function that runs every 500ms to check for changed assets, closing when activity stops
I pushed another commit to reflect implementation no. 2 (centralize asset manager in builder action). It's a smaller change, but no. 1 might be worth a refactor later.
I'll mark this draft to resolve any implementation discussions first.
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.
Here is an idea of how an awaited compiler.run
refactor might look. master...jleverenz:await-refactor
It's a bit large change, so I'll leave this PR as is, but sharing as another implementation option.
Thoughts on how to proceed?
I believe this PR #2293 should fix this issue |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What is the current behavior?
When starting an unwatched webpack compile on a project with assets (e.g. with
nest start
) the process never exits.nest build
works as expected.What is the new behavior?
It exits as expected.
Does this PR introduce a breaking change?
Other information
When starting an app with webpack, the
closeWatchers()
function is not called like it is with thetsc
compiler. This turned up for me once I switched to a monorepo project, but can be reproduced on a standard project by setting"webpack": true
in the compiler configuration along with assets.While digging into this I also noticed a difference in the way that the webpack compiler is called for the "build" vs "start" action. For "start", an
onSuccess
callback is provided (which ultimately starts the application). For "build", no callback is provided. If no callback is provided theWebpackCompiler
callscloseWatchers()
onAssetsManager
(presumably, the code is assuming it's in "build" mode). See #609The
Compiler
andWatchCompiler
don't have any references toAssetsManager
.This PR removes the functionality from
WebpackCompiler
and centralizes the behavior in the build action.