WHM support breaks projects that do not have it installed

[billyjanitsch]

After upgrading to 0.2.0, I noticed the following error:

ERROR in ./node_modules/@pmmmwh/react-refresh-webpack-plugin/src/runtime/WHMEventSource.js
Module not found: Error: Can't resolve 'webpack-hot-middleware/client' in '[...]/node_modules/@pmmmwh/react-refresh-webpack-plugin/src/runtime'

I believe this is because webpack sees this require when bundling the injected runtime code:

react-refresh-webpack-plugin/src/runtime/WHMEventSource.js

Line 7 in 0a6c114

const client = require('webpack-hot-middleware/client');

And therefore tries to resolve and bundle that module. This code path isn't taken when a WDS client has been injected, but webpack doesn't know this at bundle time. My project doesn't use WHM so that module doesn't exist on my machine.

As a temporary workaround, installing WHM (npm i -D webpack-hot-middleware) fixes the issue.

/cc @blainekasten

[blainekasten]

ahhh this is my bad. We did list this as a peerDependency. But that's probably not the right move.

My thinking is we should wrap this in a try/catch and bail if it's not installed.

[billyjanitsch]

We did list this as a peerDependency.

This would've been correct, but it was changed to an optional peer dependency in a3f792d:

react-refresh-webpack-plugin/package.json

Lines 62 to 64 in 0a6c114

	"webpack-hot-middleware": {
	"optional": true
	}

Optional peer deps are meant for cases where the package can work even if they aren't installed. It sounds like that was the original intention here, but under the current implementation, it should be a required peer dep.

My thinking is we should wrap this in a try/catch and bail if it's not installed.

I don't think that will work because require is inside of the runtime code, not the plugin. So it's a bundler error, not a runtime error, i.e. it happens while webpack is trying to resolve the runtime dependency graph via static analysis. So even if this require were inside of a try, Webpack would still try to resolve and bundle the module.

What might work is to detect the presence of the module inside of the plugin (rather than inside of the runtime), and only inject the WHM runtime into the bundle if it exists. So Webpack will only end up attempting to resolve the module if it's already been verified to exist.

This would avoid having to make WHM a required peer dep.

[pmmmwh]

This would've been correct, but it was changed to an optional peer dependency in a3f792d:

Oops - my bad.

Regarding try catch - I thought dynamic requires are supported in Webpack?

let client;
try {
  client = require('webpack-hot-middleware/client');
} catch (error) {
  // Bail
}

I'll test this later this afternoon.

[billyjanitsch]

I thought dynamic requires are supported in Webpack?

It seems that Webpack is smart enough to bundle that construct into something like this, if the required module doesn't exist at bundle time:

let client;
try {
   throw new Error('Module not found')
} catch (error) {
  // Bail
}

However it also emits a loud warning:

WARNING in ./src/index.js
Module not found: Error: Can't resolve 'webpack-hot-middleware/client' in '[...]'
 @ ./src/index.js

Do you see any disadvantage to the approach that I described of doing the try/catch in the plugin (and using it to decide which runtime to inject) rather than in the runtime?

[pmmmwh]

Do you see any disadvantage to the approach that I described of doing the try/catch in the plugin (and using it to decide which runtime to inject) rather than in the runtime?

Well, either we do it in runtime or we do it in the plugin, we will have to exhaustively detect all known/supported webpack hot integrations. This is both a good and a bad thing: the good part is that we are certain which one is being used currently, but the bad part is that if more than one exists we will have to think about how we should behave (especially when used within mono-repos). We could also make this an option, so the user can pass in which integration they are using, and we only load the runtime code for that integration (if they're using something we don't support, they could rock their own socket code as per #44 ).

[ro-savage]

@pmmmwh - Do you require a PR for this? This literally broke 0.2.0 entirely for a percentage of people.

[pmmmwh]

@pmmmwh - Do you require a PR for this? This literally broke 0.2.0 entirely for a percentage of people.

I am working on it currently, but needs a bit more time to make sure everything works fine.

[gaearon]

We could also make this an option, so the user can pass in which integration they are using, and we only load the runtime code for that integration (if they're using something we don't support, they could rock their own socket code as per #44 ).

This makes most sense to me. Don't try to guess it on the client, but let the user pick one statically in config.

[iassainov]

Still getting this issue while using webpack dev server 3.8.1.

Here are my dev-server options:

devServer: {
      contentBase: './dist',
      historyApiFallback: true,
      host: '0.0.0.0',
      port: 9002,
      overlay: {
         ....
      },
      proxy: {
         ....
      },
    },

[pmmmwh]

Still getting this issue while using webpack dev server 3.8.1.

The fix is in 0.3.0 beta. Are you using the latest release (0.2.0) or the latest beta (0.3.0-beta.5)?

[iassainov]

The fix is in 0.3.0 beta. Are you using the latest release (0.2.0) or the latest beta (0.3.0-beta.5)?

I was using 0.2.0. Thanks