Fix hanging assets watchers

[jleverenz]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

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?

[ ] Yes
[x] No

Other information

When starting an app with webpack, the closeWatchers() function is not called like it is with the tsc 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 the WebpackCompiler calls closeWatchers() on AssetsManager (presumably, the code is assuming it's in "build" mode). See #609

The Compiler and WatchCompiler don't have any references to AssetsManager.

This PR removes the functionality from WebpackCompiler and centralizes the behavior in the build action.

[jleverenz]

Repro project: https://github.com/jleverenz/nestjs-assets-hang-repro

[kamilmysliwiec]

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)

[jleverenz]

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

[jleverenz]

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

[jleverenz]

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.

[jleverenz]

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?

[kamilmysliwiec]

I believe this PR #2293 should fix this issue