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

Only add entries after compilers have been created #1774

Conversation

DylanPiercey
Copy link
Contributor

@DylanPiercey DylanPiercey commented Apr 8, 2019

  • 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
Copy link

jsf-clabot commented Apr 8, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 8, 2019

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.

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.

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

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.

Good job!

@hiroppy
Copy link
Member

hiroppy commented Apr 9, 2019

/cc @evilebottnawi added tests

@hiroppy
Copy link
Member

hiroppy commented Apr 9, 2019

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
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
Copy link
Member

hiroppy commented Apr 9, 2019

@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