fix(utils): addEntries fix #2723

knagaitsev · 2020-08-28T01:15:14Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Basically, we should not be re-injecting entries that the user provided, we should only inject webpack-dev-server entries, as the webpack compiler should already handle the user's entries

Breaking Changes

The addEntries util API has changed to take a compiler argument

Additional Info

codecov · 2020-08-28T01:20:39Z

Codecov Report

Merging #2723 (dd66b7c) into v4 (6f5d0b6) will decrease coverage by 0.19%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##               v4    #2723      +/-   ##
==========================================
- Coverage   93.04%   92.85%   -0.20%     
==========================================
  Files          39       39              
  Lines        1309     1330      +21     
  Branches      349      357       +8     
==========================================
+ Hits         1218     1235      +17     
- Misses         87       91       +4     
  Partials        4        4

Impacted Files	Coverage Δ
lib/utils/addEntries.js	`98.75% <97.29%> (-1.25%)`	⬇️
lib/utils/updateCompiler.js	`100.00% <100.00%> (ø)`
lib/utils/status.js	`80.00% <0.00%> (-8.89%)`	⬇️
lib/Server.js	`96.08% <0.00%> (-0.25%)`	⬇️
lib/utils/routes.js	`98.14% <0.00%> (-0.07%)`	⬇️
client-src/default/overlay.js	`97.01% <0.00%> (-0.05%)`	⬇️

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 6f5d0b6...572767e. Read the comment docs.

knagaitsev · 2020-09-07T00:28:01Z

lib/utils/addEntries.js

+      compiler.hooks.make.tapAsync(
+        {
+          name: 'webpack-dev-server',
+          stage: -100,


@evilebottnawi @sokra I've added the tapAsync method here, but the issue I've found now is that something like this where multiple Servers are attached to a compiler no longer works:

const compiler = webpack(config); const server1 = new Server(compiler, options1); const server2 = new Server(compiler, options2);

as it results in something like Error: Conflicting entry option static = [object Object] vs [object Object]

This worked before, as there were tests that did this which are now broken. (These tests would usually close the old server before a new one was attached)

Should we go about this by making multiple servers on one compiler discouraged, or maybe a hacky method of disabling the old hook taps like in webpack/tapable#109 after the server is closed?

Or I could also try to prevent duplicate entries by looking at the compilation globalEntry object.

I added the hacky method for disabling old hook taps, but haven't added tests for it yet

@Loonride I think we should prevent duplicate, in theory we can have property on compiler to achieve this, we use same logic here https://github.com/webpack/webpack-dev-middleware/blob/master/src/utils/setupWriteToDisk.js#L9 (but it only for webpack@4)

@evilebottnawi That is a good idea, do you think I should remove my closeCallback method and replace it with setting something like compiler.hasWebpackDevServerMakeCallback = true?

I think it is more clean hack 😄

@evilebottnawi I agree it is more clean, but one issue I now see is that if the user does:

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

If the options2 object results in a new set of entries, that will not be reflected in the compilation, since the old make callback would still be used.

alexander-akait · 2020-09-07T10:28:06Z

@Loonride ready for review?

knagaitsev · 2020-09-07T15:20:16Z

@evilebottnawi Almost, but 2 things

Need advice on the above problem
This is appearing in CI for webpack@next:

<e> [webpack-dev-middleware] TypeError: done is not a function
<e>     at ready (/home/runner/work/webpack-dev-server/webpack-dev-server/test/helpers/test-server.js:51:7)
<e>     at Hook.eval [as callAsync] (eval at create (/home/runner/work/webpack-dev-server/webpack-dev-server/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)
<e>     at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/home/runner/work/webpack-dev-server/webpack-dev-server/node_modules/webpack/node_modules/tapable/lib/Hook.js:18:14)

alexander-akait · 2020-09-07T15:25:10Z

@Loonride I think related webpack/webpack-dev-middleware#682, maybe you can provide example of configuration?

knagaitsev · 2020-09-07T15:43:59Z

It looks like the issue is here:

webpack-dev-server/test/e2e/ClientOptions.test.js

Line 404 in c7e53b0

await testServer.startAwaitingCompilation(config, testOptions);

knagaitsev · 2020-09-07T17:47:31Z

lib/Server.js

@@ -50,7 +50,7 @@ class Server {
    this.wsHeartbeatInterval = 30000;

    normalizeOptions(this.compiler, this.options);
-    updateCompiler(this.compiler, this.options);
+    this._closeCallback = updateCompiler(this.compiler, this.options);


do you agree with this naming since it is supposed to be an internal property?

knagaitsev · 2020-09-07T18:27:40Z

@evilebottnawi Ready for review

alexander-akait

I think after release webpack-dev-server@4 we should focus on extracting hot and client stuff from webpack-dev-server to separate middleware (we already have webpack-hot-middlware and webpack-client-middleware, we need deprecate them too) and use hooks for modify compiler/compilation options, also after this we can refactor webpack-dev-server to plugin, what do you think?

hiroppy · 2020-09-08T23:56:49Z

we should focus on extracting hot and client stuff from webpack-dev-server to separate middleware (we already have webpack-hot-middlware and webpack-client-middleware, we need deprecate them too)

Extracting is sounds good to me, but why do you want to deprecate them? You mean creating new libs...?

alexander-akait · 2020-09-09T10:56:02Z

@hiroppy

Extracting is sounds good to me, but why do you want to deprecate them?

Because they do the same, only in a dirty way, I think it will be easier for us to union all into the one project, it will be easier to maintain and fix

You mean creating new libs...?

Yes, new, just maybe take name, webpack-hot-middleware sounds good

knagaitsev · 2020-09-10T02:10:32Z

lib/utils/addEntries.js

+        // this invalidates old make callbacks
+        compiler.webpackDevServerMakeCallbackCount += 1;
+      } else {
+        compiler.webpackDevServerMakeCallbackCount = 1;


My idea was to make a counter for callbacks, so that only the most recent callback will be used, fixing the problem I mentioned. Let me know if this seems too hacky.

…p disabling

ylemkimon · 2020-11-17T04:07:11Z

@Loonride Would you mind if I take this PR?

alexander-akait · 2020-11-17T11:42:20Z

@ylemkimon I think you can resent this PR, based on @Loonride activity, @Loonride is not available now

if you resent this fix, I will be very thankful

Cherry-picked from webpack#2723. Co-authored-by: Loonride <knagaitsev1@gmail.com>

knagaitsev requested review from alexander-akait and hiroppy as code owners August 28, 2020 01:15

knagaitsev changed the base branch from master to v4 August 28, 2020 01:16

knagaitsev force-pushed the fix/add-entries branch from a1becda to 4db9c0a Compare September 6, 2020 21:25

knagaitsev commented Sep 7, 2020

View reviewed changes

knagaitsev closed this Sep 7, 2020

knagaitsev reopened this Sep 7, 2020

knagaitsev commented Sep 7, 2020

View reviewed changes

knagaitsev changed the title ~~WIP: addEntries fix~~ fix(utils): addEntries fix Sep 7, 2020

alexander-akait reviewed Sep 8, 2020

View reviewed changes

knagaitsev commented Sep 10, 2020

View reviewed changes

knagaitsev added 11 commits September 10, 2020 10:52

fix(server): work on fixing addEntries

ae5b765

fix(entry): handle webpack 5 addEntries as plugin

a07a914

chore(server): create prependEntries util

317617a

chore(server): fix how addEntries works for multi compiler

438f6a4

test(server): add module federation test

e7eae8b

test(utils): working on addEntries new tests

ee0c95b

test(util): finish addEntries tests

ca8abc6

chore(server): move prependEntries util back where it was

0cb08c8

test(server): update server test for webpack 5

6859414

chore(server): add close callback

833e70f

chore(server): change how close callback works

0a15f2f

knagaitsev added 6 commits September 10, 2020 10:52

test(server): add async start for test server

1a91e22

test(server): remove error logging for transportMode test

82efa72

chore(server): change name of close callback and add comment about ta…

ce3a419

…p disabling

test(server): add tests for close callback of addEntries

ebd1e60

chore(server): remove close callback make callback in favor of counter

6de37f2

chore(deps): downgrade back to webpack@4

dd66b7c

knagaitsev force-pushed the fix/add-entries branch from 3207c7d to dd66b7c Compare September 10, 2020 15:55

alexander-akait closed this Oct 24, 2020

alexander-akait reopened this Oct 24, 2020

Merge branch 'v4' into fix/add-entries

572767e

ylemkimon added a commit to ylemkimon/webpack-dev-server that referenced this pull request Nov 17, 2020

test(integration): add module federation test

d5089ec

Cherry-picked from webpack#2723. Co-authored-by: Loonride <knagaitsev1@gmail.com>

ylemkimon mentioned this pull request Nov 17, 2020

fix(utils): use plugin to add entries for webpack@5 #2839

Merged

6 tasks

alexander-akait closed this Nov 27, 2020

alexander-akait deleted the branch webpack:v4 November 27, 2020 15:05

ylemkimon mentioned this pull request Feb 19, 2021

webpack compiler cannot be reused across server instances #3033

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(utils): addEntries fix #2723

fix(utils): addEntries fix #2723

knagaitsev commented Aug 28, 2020 •

edited

codecov bot commented Aug 28, 2020 •

edited

knagaitsev Sep 7, 2020

knagaitsev Sep 7, 2020

knagaitsev Sep 7, 2020

alexander-akait Sep 8, 2020

knagaitsev Sep 8, 2020

alexander-akait Sep 8, 2020

knagaitsev Sep 10, 2020

alexander-akait commented Sep 7, 2020

knagaitsev commented Sep 7, 2020

alexander-akait commented Sep 7, 2020

knagaitsev commented Sep 7, 2020

knagaitsev Sep 7, 2020

alexander-akait Sep 7, 2020

knagaitsev commented Sep 7, 2020

alexander-akait left a comment

hiroppy commented Sep 8, 2020

alexander-akait commented Sep 9, 2020

knagaitsev Sep 10, 2020

ylemkimon commented Nov 17, 2020

alexander-akait commented Nov 17, 2020 •

edited

fix(utils): addEntries fix #2723

fix(utils): addEntries fix #2723

Conversation

knagaitsev commented Aug 28, 2020 • edited

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Aug 28, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Sep 7, 2020

knagaitsev commented Sep 7, 2020

alexander-akait commented Sep 7, 2020

knagaitsev commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Sep 7, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

hiroppy commented Sep 8, 2020

alexander-akait commented Sep 9, 2020

Choose a reason for hiding this comment

ylemkimon commented Nov 17, 2020

alexander-akait commented Nov 17, 2020 • edited

knagaitsev commented Aug 28, 2020 •

edited

codecov bot commented Aug 28, 2020 •

edited

alexander-akait commented Nov 17, 2020 •

edited