refactor(utils): addEntries to DevServerPlugin

[ylemkimon]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

#2841 (comment)

Removed DevServerEntryPlugin and moved addEntries into DevServerPlugin.

Breaking Changes

Yes. lib/utils/addEntries (exported as Server.addDevServerEntrypoints) function is now lib/utils/DevServerPlugin, a webpack plugin and no longer exported. It now takes a webpack-dev-server option as a constructor argument and deduplication should be handled by including only a single instance of the plugin.

const webpack = require('webpack');
const webpackDevServer = require('webpack-dev-server');

const compiler = webpack(config);
webpackDevServer.addDevServerEntrypoints(compiler, devServerOptions);

should be changed to

const webpack = require('webpack');
const devServerPlugin = require('lib/utils/DevServerPlugin');

if (!config.plugins.find((p) => p.constructor === devServerPlugin)) {
  config.plugins.push(new devServerPlugin(devServerOptions));
}
const compiler = webpack(config);

Furthermore, using HotModuleReplacementPlugin from another webpack installation is not supported and should be avoided.

Additional Info

All changes are refactoring, so ignoring whitespace would help reviewing the diff.

[codecov]

Codecov Report

Merging #2844 (7c4af08) into v4 (6c2082f) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##               v4    #2844      +/-   ##
==========================================
- Coverage   93.08%   93.02%   -0.07%     
==========================================
  Files          40       39       -1     
  Lines        1331     1305      -26     
  Branches      354      355       +1     
==========================================
- Hits         1239     1214      -25     
+ Misses         88       87       -1     
  Partials        4        4              

Impacted Files	Coverage Δ
lib/Server.js	96.32% <ø> (-0.01%)	⬇️
lib/utils/DevServerPlugin.js	100.00% <100.00%> (ø)
lib/utils/updateCompiler.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c2082f...7c4af08. Read the comment docs.

[alexander-akait]

Anyway good job and big thanks

[alexander-akait]

Looks good, let's rename file and I think we can merge it, good job and big thanks again, you are great ⭐

[alexander-akait]

Forget about one - do we have multi compiler tests?

[ylemkimon]

Forget about one - do we have multi compiler tests?

webpack-dev-server/test/server/utils/updateCompiler.test.js

Lines 142 to 151 in 17b6500

	describe('multi compiler config, hot', () => {
	let multiCompiler;
	beforeAll(() => {
	const webpackConfig = require('../../fixtures/multi-compiler-2-config/webpack.config');
	webpackConfig[1].plugins = [new webpack.HotModuleReplacementPlugin()];
	multiCompiler = webpack(webpackConfig);
	});

[ylemkimon]

I cannot seem to fix type errors. If I require('./getSocketClientPath') in addEntries, it looks inside and type-checks client source.

If I add a module declaration file (.d.ts) for getSocketClientPath, it'd not look inside client sources. (Tried jsdoc, but it still looks inside client sources)

[alexander-akait]

I found better way to do it in webpack@5, let's check:

const isWebTarget = compiler.optons.externalsPresets 
  ? compiler.optons.externalsPresets.web
  : // Old logic for webpack@4

[ylemkimon]

Should we add client entries when compiler.optons.externalsPresets.web === null, i.e., targets are mixed, e.g., ['web', 'node']?

[alexander-akait]

No, only when it is web, if it is not web and still in browser (I don't know this case 😄 ) you should use injectHot: true

[alexander-akait]

Very good job, only one improvement and we can merge it

[alexander-akait]

Let's fix tests and merge it

[alexander-akait]

Just apply no need to push it in plugins array

[ylemkimon]

If it is not pushed to plugins, it'd add it again if the plugin is applied again.

[alexander-akait]

Can you clarify? We should not touch compilerOptions, only use plugin.apply(compiler)

[ylemkimon]

If newly applied plugins are not added to config.plugins, then in

const compiler = webpack(config);
const server1 = new Server(compiler, options1); // calls devServerPlugin.apply(compiler)
server1.close(() => {
  const server2 = new Server(compiler, options2); // calls devServerPlugin.apply(compiler), again
});

each time the Server constructor is called, HMR plugin and this plugin will be applied. So they will be applied twice, causing duplicate hooks and entries.

[ylemkimon]

I think using a single compiler instance across multiple dev-server instances should be reconsidered, as it's impossible to remove or update hooks and it's not possible to persist state (whether plugins and hooks are added, etc.) without touching compiler or compiler.options. It seems it was not properly supported before.

[alexander-akait]

I think here is the architecture problem, if we will be plugin we will not have this problem, so we need focus for v5 on refactor to plugin

[alexander-akait]

We can mark it as limitation now

[ylemkimon]

Removed compilerOptions.plugins.push(plugin). This PR is ready to merge.

[ylemkimon]

The plugin would have the same limitation, since the second dev-server (server2) cannot get or modify the plugin added by the first dev-server (server1). Do you mean refactoring the whole package into a plugin?

[alexander-akait]

Good job and big thanks, we need focus on CLI, it is the last issue (hope)

[alexander-akait]

We need remove cli logic, because it is deprecated and no longer work with webpack-cli, so we need remove this, but for better DX maybe we can keep CLI file, but what we will do:

• throw friendly error about migrate on webpack serve
• run webpack serve by default as webpack do https://github.com/webpack/webpack/blob/master/bin/webpack.js

What do you think?

[ylemkimon]

How about we defer (delegate) to webpack:

// maybe throw a friendly error if webpack is not installed
process.argv.splice(2, 0, 'serve');
require('webpack/bin/webpack');

?

[alexander-akait]

Sounds good, but am afraid webpack can change logic too in future and we will can broken again, so I think it would be better to use webpack-cli directly, yep copy/paste code, but it protected us from somethings unexpected