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.

Sign up for GitHub

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

Jump to bottom

Fix hanging assets watchers #1922

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix hanging assets watchers #1922

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02fbce3
fix: close file watchers after unwatched webpack compile
jleverenz Feb 7, 2023
36df275
refactor: remove extra 'closeWatchers' from compiler
jleverenz Feb 7, 2023
d329e3e
fix: Listen for webpack competion before calling closeWatchers
jleverenz Feb 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions actions/build.action.ts
View file
Open in desktop
Expand Up @@ -122,16 +122,21 @@ export class BuildAction extends AbstractAction {
webpackPath,
configuration.compilerOptions!.webpackConfigPath!,
);
return this.webpackCompiler.run(
this.webpackCompiler.run(
configuration,
webpackConfigFactoryOrConfig,
pathToTsconfig,
appName,
isDebugEnabled,
watchMode,
this.assetsManager,
onSuccess,
);

if (!watchMode) {
this.assetsManager.closeWatchers();
}

return;
Copy link
Member

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)

Copy link
Author

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

Copy link
Author

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

Copy link
Author

@jleverenz jleverenz Feb 11, 2023 •
edited

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 of tsc, 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.

Copy link
Author

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?

}

if (watchMode) {
Expand Down
5 changes: 1 addition & 4 deletions lib/compiler/webpack-compiler.ts
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ export class WebpackCompiler {
appName: string,
isDebugEnabled = false,
watchMode = false,
assetsManager: AssetsManager,
onSuccess?: () => void,
) {
const cwd = process.cwd();
Expand Down Expand Up @@ -130,9 +129,7 @@ export class WebpackCompiler {
assets: false,
});
if (!err && !stats!.hasErrors()) {
if (!onSuccess) {
assetsManager.closeWatchers();
} else {
if (onSuccess) {
onSuccess();
}
} else if (!watchMode && !watch) {
Expand Down