pmmmwh / react-refresh-webpack-plugin

A Webpack plugin to enable "Fast Refresh" (also previously known as Hot Reloading) for React components.
MIT License
3.14k stars 193 forks source link

webpack version detection should use feature detection #308

Closed ndelangen closed 3 years ago

ndelangen commented 3 years ago

the webpack version is determined using a require, but this is problematic in monorepos where the location of this plugin and the location of webpack can be unexpected.

Is there some way to infer the version we're dealing with from the compiler passed into apply?

ndelangen commented 3 years ago

Or resolve-from the compiler.context?

pmmmwh commented 3 years ago

The issue with feature detection is that we need to import stuff from Webpack that is only on a certain version. If require('webpack') does not work reliably, the imports we do (like require('webpack/lib/RuntimeModule')) would also not work reliably?

ndelangen commented 3 years ago

would it be possible to provide the path to webpack to the constructor?

pmmmwh commented 3 years ago

Hmm, that would be possible, but is it possible to describe the use cases and current limitations to me so I could understand the situation more?

ndelangen commented 3 years ago

I'm trying to add webpack 5 support for storybook whilst also still allowing webpack 4 support. storybook has thus has a dependency on both, making it impossible to exactly predict where which version will be installed, on the user's system. In our monorepo, webpack 5 is installed at the root. and so is react-refresh-webpack-plugin. resulting in react-refresh-webpack-plugin always behaving as if it was ran with webpack 5, even if it's actually running webpack 4.

phaistonian commented 3 years ago

@ndelangen I am right there at the moment; trying to make the new storybook alpha work with react refresh.

It seems that the culprit is compilation.hooks.additionalTreeRuntimeRequirements.tap.

@pmmmwh Is there way to force using webpack 5 or 4 and override the automatic detection?

pmmmwh commented 3 years ago

Is there way to force using webpack 5 or 4 and override the automatic detection?

Not currently. I would be open to add that, but it would also be quite clear that you're (mostly) on your own if you opt into that.

storybook has thus has a dependency on both

Can I ask more about why not opt for resolutions or aliases (in a monorepo), or for peer dependencies on Webpack? How is the dependency chain actually configured and why would Node not resolve properly?

phaistonian commented 3 years ago

@pmmmwh I tried using yarn resolutions and I encountered a number of errors — not recall what those were, to be honest.

My guess is that since Storybook uses a hybrid model in order to support both Webpack 4 and 5, somehow confusion comes into play.

I will give it another try today and I come up with more context I will follow up here.

ndelangen commented 3 years ago

We can't enforce a single version of webpack in storybook, because we use 1 (fixed) version of webpack to build the manager, and then the user can control what version of webpack to build the preview with.

rdsedmundo commented 3 years ago

I'm also facing this using pnpm package manager in a monorepo. I have webpack@5 installed in the workspace root that is used for backend projects, but for a CRA app (that also uses Storybook) it's still on webpack v4 as CRA hasn't landed support for it yet.

The react-refresh plugin is trying to import the wrong version of webpack (5) and failing with:

(node:28718) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'tap' of undefined

at

compilation.hooks.additionalTreeRuntimeRequirements.tap(

I work around it by forcing pnpm to add webpack@4 as a dependency of react-refresh:

function readPackage(pkg) {
  // TODO: webpack5 https://github.com/facebook/create-react-app/issues/9994
  if (
    pkg.name === '@pmmmwh/react-refresh-webpack-plugin' &&
    pkg.peerDependencies?.webpack?.startsWith('>=4')
  ) {
    pkg.dependencies = {
      ...pkg.dependencies,
      webpack: '4.46.0',
    };
  }

  return pkg;
}

module.exports = {
  hooks: {
    readPackage,
  },
};

See also: https://github.com/pnpm/pnpm/discussions/3437

pmmmwh commented 3 years ago

I think the main problem that I'm unsure here is how should we pull in files when require does not work reliably.

It is quite easy to make the plugin to detect current version on the compiler, but the difference between Webpack 4 and Webpack 5 is larger that we would need to require Webpack 5 specific files, which means we are again dependent on require working properly. One solution mentioned above would be to allow you provide the path to Webpack, but it would make things quite complicated and I'm not sure if that's the best approach.

ndelangen commented 3 years ago

Webpack should dependency inject webpack into the Plugin.apply call, maybe it already does?

rdsedmundo commented 3 years ago

What are the drawbacks of doing dynamic requires as needed? Like this (pseudocode):

let moduleInfo;

if (compiler.version === 5) {
  moduleInfo = require('webpack/lib/moduleInfo');
} else if (compiler.version === 4) {
  const compilation = require('webpack/lib/compilation');
  moduleInfo = compilation.stats.modules.info;
}

moduleInfo.forEach(...)

I see that you're already doing this in some places, and if you say that it's easy to detect webpack's version from the compiler object, I'm not sure if I got what is blocking us here then.

ndelangen commented 3 years ago

Consider this package getting hoised to the top of a monorepo, Webpack 5 is also hoisted to the top.

Now a sub-package in this monorepo depends on webpack 4 and runs it, and uses this plugin.

Webpack 4 is adding this addon, which will require webpack (will require webpack 5, because it's hoisted). Now webpack 4 throws an error, because the plugin uses webpack 5 api's.


You can simply not assume that the webpack version you require is the same as the webpack version your plugin is invoked with.

pmmmwh commented 3 years ago

I'm not sure if I got what is blocking us here then.

The issue is the require does not work reliably - even if I version detect correctly from the compiler instance, requiring Webpack 5 specific paths will still throw with file not found.

rdsedmundo commented 3 years ago

Now I understand, thank you all for the explanations. I assume this is a quite common scenario for loaders and plugins, we could see what others are doing. I'm not sure if they rely on require or if they pick what they need from the compiler object. One scenario that I've seen in the past is to use separate branches and major versions in NPM for supporting different versions of webpack.

@alexander-akait @sokra by any chance could you give any tips for us here? TLDR: we have multiple versions of webpack in the node_modules from a monorepo, and even though we call the plugin using webpack@4, inside the plugin code require('webpack') returns webpack@5, which confuses the plugin and makes it use features that aren't available on the webpack version it was called from.

alexander-akait commented 3 years ago

For webpack v5 please use compier.webpack https://github.com/webpack/webpack/blob/master/types.d.ts#L1814 to get webpack exports, for v4 compatibility you can use const webpack = compier.webpack || require('webpack');

pmmmwh commented 3 years ago

TIL I can do compiler.webpack.

I guess this solves the issue of having WP4 hoisted and wanted to use WP5, but if the hoisted version is WP5 and the local version is WP4 this would sill break as there is a require call ...

I'll try to test this out.

alexander-akait commented 3 years ago

compiler.webpack fix the problem when webpack v4 on top, this is a more common problem, than versa vice. Also in future to avoid this problem better always use compiler.webpack, we migrate on this in all official plugins and loaders

pmmmwh commented 3 years ago

Yep I think this would solve most if not all of our problems. Thanks for chiming in!

Is there any lower-bound version for WP5 to use compiler.webpack?

alexander-akait commented 3 years ago

5.1, but I think using const webpack = compiler.webpack || require('webpack'); solves the problems, in future dropping webpack v4 solve the whole problem

ndelangen commented 3 years ago

in future dropping webpack v4 solve the whole problem

For a while, there will always be new versions in the future, and the hoisting problem will be there always.

alexander-akait commented 3 years ago

Also if you use webpack-cli, we have WEBPACK_PACKAGE env variable (https://github.com/webpack/webpack-cli/blob/master/packages/webpack-cli/lib/webpack-cli.js#L12), so you can set webpack package location

pmmmwh commented 3 years ago

Is there a way to access JavascriptParserHelpers? It doesn't seem to be available under the compiler.

Edit: Hmm, never mind, tapping through DefinePlugin seems to work as well.

pmmmwh commented 3 years ago

Hi @ndelangen @rdsedmundo -

I've implemented proper version detection and pass-through imports on the compiler instance for Webpack 5 in #415. Would you mind testing it out in your projects?

npm install -D -S git://github.com/pmmmwh/react-refresh-webpack-plugin.git#feat/feature-detection
yarn add -D git://github.com/pmmmwh/react-refresh-webpack-plugin.git#feat/feature-detection
pnpm add -D git://github.com/pmmmwh/react-refresh-webpack-plugin.git#feat/feature-detection

I'll merge that once I got around to fill out tests for the changes and we can confirm that the PR works in reality.

rdsedmundo commented 3 years ago

It worked for my project. Thanks for working on that! 🎉