Skip to content

Only add entries after compilers have been created#1774

Merged
alexander-akait merged 2 commits into
webpack:masterfrom
DylanPiercey:only-add-entries-after-compiler-created
Apr 9, 2019
Merged

Only add entries after compilers have been created#1774
alexander-akait merged 2 commits into
webpack:masterfrom
DylanPiercey:only-add-entries-after-compiler-created

Conversation

@DylanPiercey

@DylanPiercey DylanPiercey commented Apr 8, 2019

Copy link
Copy Markdown
Contributor
  • 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?

No...

Motivation / Use-Case

Currently addEntries is called twice when using the cli. First it is called here in the cli bin, and then always called again when we create the dev server instance by calling updateCompiler then here.

This extra call is not needed, and actually causes an issue with my specific setup. In my Webpack config the entry is set dynamically via a plugin once the plugin has initialized. Since the first call to addEntries is ran on the raw config before any plugins run it is inaccurate and thinks that I have no entries, causing some issues. This would cause issues for any plugin which mutates the entries assuming it is used with the webpack-dev-server cli.

Additional Info

If a test case is required for this then I'd be happy to work with someone to figure out the best way to integrate such a test into this suite. It looks like the CLI tests are fairly basic and separated from the server tests.

@jsf-clabot

jsf-clabot commented Apr 8, 2019

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Apr 8, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1774 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1774   +/-   ##
=======================================
  Coverage   87.68%   87.68%           
=======================================
  Files           9        9           
  Lines         593      593           
  Branches      177      177           
=======================================
  Hits          520      520           
  Misses         61       61           
  Partials       12       12

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 66b04a9...4bf5d96. Read the comment docs.

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, looks we forget about remove this line, we need tests. We have regression after release 3.3.0 so i do release today, If you have time to add tests it will be good, otherwise I will write them myself

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good job!

@hiroppy

hiroppy commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

/cc @evilebottnawi added tests

@hiroppy

hiroppy commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

Oops, I'll fix sep for windows.

@hiroppy hiroppy force-pushed the only-add-entries-after-compiler-created branch from 9db03d8 to 468e45f Compare April 9, 2019 18:21
@alexander-akait

Copy link
Copy Markdown
Member

/cc @hiroppy need rebase 😞

@hiroppy hiroppy force-pushed the only-add-entries-after-compiler-created branch from 468e45f to 4bf5d96 Compare April 9, 2019 18:35
@hiroppy hiroppy requested a review from alexander-akait April 9, 2019 18:36
@hiroppy

hiroppy commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

@evilebottnawi PTAL;)

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.

4 participants