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

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

Merged
merged 5 commits into from
Nov 17, 2020

Conversation

ylemkimon
Copy link
Contributor

@ylemkimon ylemkimon commented Nov 17, 2020

  • 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, added module federation test cherry-picked from #2723.

Motivation / Use-Case

Fixes #2484 and fixes #2692. Closes #2723.

Instead of modifying compiler configuration and calling hooks.entryOption after it has instantiated, this calls compilation.addEntry to add dev server entries into globalEntry, as suggested in #2692 (comment).

As discussed in #2723 (review), the issue is with reusing the compiler instance with multiple dev servers, so I've decided to use a plugin, whose entries can be modified later.

I've considered using existing plugins:

  • EntryPlugin: adds a single entry, so changing the number of entries is not possible because removing hooks is not supported
  • DynamicEntryPlugin: adding a global entry, i.e., name: undefined, is not possible because name should be a string (https://git.io/JkleI)

So I've created DevServerEntryPlugin based on DynamicEntryPlugin. It would also allow easy detection.

Breaking Changes

The signature of webpack-dev-server/lib/utils/addEntries has changed from addEntries(config, options, server) to addEntries(compiler, options, server), but GitHub code search shows no code is using this directly.

Additional Info

The first three commits are revert or refactor. Diff ignoring whitespace changes would be better for reviewing changes.

@ylemkimon ylemkimon changed the title Add entries fix(utils): use plugin to add entries for webpack@5 Nov 17, 2020
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #2839 (d5089ec) into v4 (6f5d0b6) will increase coverage by 0.03%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v4    #2839      +/-   ##
==========================================
+ Coverage   93.04%   93.08%   +0.03%     
==========================================
  Files          39       40       +1     
  Lines        1309     1331      +22     
  Branches      349      354       +5     
==========================================
+ Hits         1218     1239      +21     
- Misses         87       88       +1     
  Partials        4        4              
Impacted Files Coverage Δ
lib/utils/DevServerEntryPlugin.js 94.11% <94.11%> (ø)
lib/utils/addEntries.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 6f5d0b6...d5089ec. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very great idea

@alexander-akait
Copy link
Member

Some tests are still flaky 😞

@alexander-akait alexander-akait merged commit 21bc47a into webpack:v4 Nov 17, 2020
@alexander-akait
Copy link
Member

Big thanks

@alexander-akait
Copy link
Member

@ylemkimon Now we need focus on target: #2761, then on CLI (drop old code and get code from webpack, when you run webpack-dev-server we should run webpack-cli, now we throw an error) + docs (webpack serve) and I think we will do release

@ylemkimon
Copy link
Contributor Author

@evilebottnawi Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants