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

Warnings + error messages with hot reload + yarn v2 #5928

Closed
brandon-leapyear opened this issue Dec 11, 2020 · 16 comments
Closed

Warnings + error messages with hot reload + yarn v2 #5928

brandon-leapyear opened this issue Dec 11, 2020 · 16 comments
Labels
needs triage This issue has not been looked into

Comments

@brandon-leapyear
Copy link

brandon-leapyear commented Dec 11, 2020

Bug Report

Adding hot reload with yarn 2 in a new nest project runs the nest server successfully (i.e. curl localhost:3000 returns "Hello world"), but the output shows a deprecation warning and error messages related to requiring uninstalled packages.

Repro:

  1. nest new
  2. yarn set version berry && yarn
  3. Add hot reload
    1. Follow instructions per https://docs.nestjs.com/recipes/hot-reload
    2. Replace start-server-webpack-plugin with run-script-webpack-plugin (Please upgrade to Webpack 5 ericclemmons/start-server-webpack-plugin#40 (comment))
    3. Fix WatchIgnorePlugin (docs(hot-reload): update hot-reload.md docs.nestjs.com#1568)
    4. Run yarn add -D webpack@5.9.0 (Move webpack to peerDependency for Yarn v2 + hot reload nest-cli#992)
  4. yarn start:dev

Current behavior

You'll see one deprecation warning:

(node:35944) [DEP_WEBPACK_WATCH_WITHOUT_CALLBACK] DeprecationWarning: A 'callback' argument need to be provided to the 'webpack(options, callback)' function when the 'watch' option is set. There is no way to handle the 'watch' option without a callback.
(Use `node --trace-deprecation ...` to show where the warning was created)

and errors like

ERROR in ./.yarn/unplugged/@nestjs-core-virtual-330e9c6eb9/node_modules/@nestjs/core/nest-application.js 16:102-145
Module not found: Error: @nestjs/core tried to access @nestjs/websockets (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.

Required package: @nestjs/websockets (via "@nestjs/websockets/socket-module")
Required by: @nestjs/core@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1 (via /Users/bchinn/Desktop/yarn2/nest-hrm-yarn2/.yarn/unplugged/@nestjs-core-virtual-330e9c6eb9/node_modules/@nestjs/core/)
Ancestor breaking the chain: @nestjs/platform-express@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1
Ancestor breaking the chain: @nestjs/testing@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1
Ancestor breaking the chain: foo@workspace:.


 @ ./.yarn/unplugged/@nestjs-core-virtual-330e9c6eb9/node_modules/@nestjs/core/index.js 25:21-50
 @ ./src/main.ts 3:15-38

Now #5477 correctly added these packages to peerDependencies and specified them as optional with peerDependenciesMeta, but I think the way NestJS tries to load optional dependencies (i.e. try-catch require) triggers an error message with Yarn v2.

const {
MicroservicesModule,
} = optionalRequire('@nestjs/microservices/microservices-module', () =>
require('@nestjs/microservices/microservices-module'),
);

export function optionalRequire(packageName: string, loaderFn?: Function) {
try {
return loaderFn ? loaderFn() : require(packageName);
} catch (e) {
return {};
}
}

Input Code

See repro above

Expected behavior

yarn start:dev should not show any deprecation warnings or error messages, the way it displays without the "Add hot reload" step in the repro.

Possible Solution

Some possible solutions:

  1. Only require optional dependencies when they're actually used
  2. Implement optionalRequire differently, possibly using the PnP API to check if a package is installed before trying to require it

Environment


Nest version: X.Y.Z

 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

@brandon-leapyear brandon-leapyear added the needs triage This issue has not been looked into label Dec 11, 2020
@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 11, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


But also, more generally speaking, I would expect

nest build --webpack --webpackPath webpack.config.js

with

module.exports = function(options) {
  return options
}

to be equivalent to nest build, but nest build finishes successfully, while it errors with a noop webpack.config.js

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

Hi, @brandon-leapyear

Try doing this:

  • nest n appor nest nest app

  • Package manger uses yarn

  • yarn set version berry && yarn If you want to use version 2 of yarn

  • yarn add webpack-node-externals start-server-nestjs-webpack-plugin --dev

  • Create file webpack-hmr.config.jsin the project root

I used this:

const webpack = require('webpack');
const nodeExternals = require('webpack-node-externals');
const StartServerPlugin = require('start-server-nestj-webpack-plugin');

module.exports = function(options) {
  return {
    ...options,
    entry: ['webpack/hot/poll?100', options.entry],
    externals: [
      nodeExternals({
        allowlist: ['webpack/hot/poll?100'],
      }),
    ],
    plugins: [
      ...options.plugins,
      new webpack.HotModuleReplacementPlugin(),
      new webpack.WatchIgnorePlugin({
          paths: [/\.js$/, /\.d\.ts$/]
      }),
      new StartServerPlugin({ name: options.output.filename }),
    ],
  };
};
  • Open the package.json and edit the yarn run start:dev script in :
    "start:dev": "nest build --webpack --webpackPath webpack-hmr.config.js"

and then from the terminal do: yarn run start:dev

@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 12, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


@Tony133 what would that do differently? I already included in my repro instructions the step of replacing start-server-webpack-plugin with run-script-webpack-plugin

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

I tried as I wrote you above and it works if you want to try

at the most if you want to turn the test repository

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

the problem if I'm not mistaken is at this point:

ii. Replace start-server-webpack-plugin with run-script-webpack-plugin

you should replace: start-server-webpack-plugin with start-server-nestjs-webpack-plugin and not with "run-script-webpack-plugin"

@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 12, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


@Tony133 That's not the issue. run-script-webpack-plugin is basically the same as start-server-nestjs-webpack-plugin. Are you sure you tried it with Yarn 2? Yarn 2 does things differently.

So I actually got it working by replacing webpack-node-externals with webpack-pnp-externals. In summary, the following instructions get hot-reload to work with yarn 2 (modulo deprecation warnings):

  1. yarn add -D webpack-pnp-externals run-script-webpack-plugin webpack@5.9.0 (completely replaces the existing npm install instruction in the docs)
  2. Add the following webpack-hmr.config.js file:
    const webpack = require('webpack');
    const nodeExternals = require('webpack-node-externals');
    const { RunScriptWebpackPlugin } = require('run-script-webpack-plugin');
    const { WebpackPnpExternals } = require('webpack-pnp-externals');
    
    module.exports = function(options) {
      return {
        ...options,
        entry: ['webpack/hot/poll?100', options.entry],
        watch: true,
        externals: [
          WebpackPnpExternals({ exclude: ['webpack/hot/poll?100'] }),
        ],
        plugins: [
          ...options.plugins,
          new webpack.HotModuleReplacementPlugin(),
          new webpack.WatchIgnorePlugin({
            paths: [/\.js$/, /\.d\.ts$/]
          }),
          new RunScriptWebpackPlugin({ name: options.output.filename }),
        ],
      };
    };
    The differences with the webpack-hmr.config.js in the docs are:
    • Use RunScriptWebpackPlugin instead of StartServerWebpackPlugin (StartServerWebpackPlugin incompatible with Webpack 5)
    • Fix WatchIgnorePlugin to use paths key
    • Use WebpackPnpExternals instead of webpack-node-externals (yarn 2 does not have a node_modules directory)
  3. Change start:dev script as usual

@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 12, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


In total, to resolve this issue, I'd recommend adding the above instructions to the NestJS docs about Hot Reload for Yarn v2, as well as fixing the DEP_WEBPACK_WATCH_WITHOUT_CALLBACK deprecation message (i believe this happens even without Yarn v2)

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

For this point:
"Use RunScriptWebpackPlugin instead of StartServerWebpackPlugin (StartServerWebpackPlugin incompatible with Webpack 5"
I install: start-server-nestjs-webpack-plugin

@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 12, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


@Tony133 I understand. Again, that's not the issue here. I personally prefer run-script-webpack-plugin, but either works. I do recommend NestJS docs say run-script-webpack-plugin, to avoid having special dependencies that are special to NestJS

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

Ok, I do some tests then I'll let you know

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

@brandon-leapyear For this warning:

(node:35944) [DEP_WEBPACK_WATCH_WITHOUT_CALLBACK] DeprecationWarning: A 'callback' argument need to be provided to the 'webpack(options, callback)' function when the 'watch' option is set. There is no way to handle the 'watch' option without a callback.
(Use `node --trace-deprecation ...` to show where the warning was created)

Just remove:

watch: true

reference: --> webpack/webpack-cli#1918

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

The documentation must also be updated for the yarn I did a shot as you have seen and proposed to specify it better in the documentation.

I could also use that pull to update the documentation

I add here the reference to the pull to connect the speech: ---> nestjs/docs.nestjs.com#1568

@brandon-leapyear
Copy link
Author

brandon-leapyear commented Dec 12, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


Just remove watch: true

But... I want watch: true for hot reload

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

instead for this mistake:

ERROR in ./.yarn/unplugged/@nestjs-core-virtual-330e9c6eb9/node_modules/@nestjs/core/nest-application.js 16:102-145
Module not found: Error: @nestjs/core tried to access @nestjs/websockets (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.

Required package: @nestjs/websockets (via "@nestjs/websockets/socket-module")
Required by: @nestjs/core@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1 (via /Users/bchinn/Desktop/yarn2/nest-hrm-yarn2/.yarn/unplugged/@nestjs-core-virtual-330e9c6eb9/node_modules/@nestjs/core/)
Ancestor breaking the chain: @nestjs/platform-express@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1
Ancestor breaking the chain: @nestjs/testing@virtual:6847a7e26c6f65e9a07a05ac6fd51a817f487ca65a3f4eb6074a6e87e9e5a9b8d604589ccca3616c3ade1ca80766e0dab48ed3499f9be6e35afa8028ddc2857f#npm:7.6.1
Ancestor breaking the chain: foo@workspace:.


to me it doesn't

@Tony133
Copy link
Contributor

Tony133 commented Dec 12, 2020

@brandon-leapyear does removing watch: true still give you the warning?

however also on npm from this warning, not only with yarn 2

reference: --> webpack/webpack-cli#1918

@kamilmysliwiec
Copy link
Member

We're tracking Hot Reload regression here nestjs/docs.nestjs.com#1554

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

3 participants