Skip to content

feat(webpack-dev-server): add WEBPACK_DEV_SERVER environment variable #1533

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

Closed
wants to merge 1 commit into from

Conversation

donaldpipowitch
Copy link

@donaldpipowitch donaldpipowitch commented Oct 18, 2018

  • This is a bugfix
  • This is a code refactor
  • This is a test update
  • This is a typo fix
  • This is a metadata update
  • This is a feature ← It looks like this is missing in the template? ⚠️

For Bugs and Features; did you add new tests?

No. I just added it in the same way webpack-serve did it and they had no test for this either 🤔As this was an official package before I assume it's okay for this one as well?

Motivation / Use-Case

As described here: #1532.

Breaking Changes

No.

Additional Info

I placed this just before require('webpack-cli/bin/convert-argv') which will require my config and this env variable should be visible for the config file.

👋

@codecov
Copy link

codecov bot commented Oct 18, 2018

Codecov Report

Merging #1533 into master will decrease coverage by 13.87%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1533       +/-   ##
===========================================
- Coverage   89.19%   75.32%   -13.88%     
===========================================
  Files           9       10        +1     
  Lines         611      689       +78     
  Branches      186        0      -186     
===========================================
- Hits          545      519       -26     
- Misses         54      170      +116     
+ Partials       12        0       -12
Impacted Files Coverage Δ
lib/Server.js 81.81% <100%> (-3.51%) ⬇️
bin/webpack-dev-server.js 58.52% <100%> (ø)
lib/utils/createLogger.js 87.5% <0%> (-12.5%) ⬇️
lib/utils/addEntries.js 100% <0%> (ø) ⬆️
lib/utils/createCertificate.js 100% <0%> (ø) ⬆️
lib/utils/createDomain.js 100% <0%> (ø) ⬆️
lib/utils/updateCompiler.js
lib/utils/defaultTo.js
lib/utils/findPort.js
... and 6 more

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 4c52153...70e2600. 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.

Not right place, we should set WEBPACK_DEV_SERVER when you run using node api too, your code set env var only when you run dev server using cli. Here right place https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L60

@donaldpipowitch
Copy link
Author

Then I'd set it in both places, because new Server is called too late, if I use the CLI API, if I'm not mistaken. The custom config file would already be required at that stage.

@alexander-akait
Copy link
Member

@donaldpipowitch too late? Can you describe where it is late?

@alexander-akait
Copy link
Member

Also need add tests to avoid regressions in future

@donaldpipowitch
Copy link
Author

donaldpipowitch commented Oct 18, 2018

Can you describe where it is late?

We need this env var, before the webpack.config.js is required. If I'm not mistaken this happens before new Server() in case of the CLI.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 18, 2018

@donaldpipowitch looks you right, please add test, also fix lint problem

@michael-ciniawsky michael-ciniawsky changed the title fixes #1532 - set WEBPACK_DEV_SERVER env var feat(webpack-dev-server): add support for WEBPACK_DEV_SERVER env var Oct 18, 2018
@donaldpipowitch
Copy link
Author

Added lint fix and a small test. 👍

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.

Need test for Server too

@donaldpipowitch
Copy link
Author

I wasn't sure where I should put that test, so I created a new file. Hope that's fine?

@alexander-akait
Copy link
Member

Hm, CI tests on windows broken 😕 Not related to this PR

@michael-ciniawsky michael-ciniawsky changed the title feat(webpack-dev-server): add support for WEBPACK_DEV_SERVER env var feat(webpack-dev-server): add WEBPACK_DEV_SERVER environment variable Oct 23, 2018
@donaldpipowitch
Copy link
Author

Anything more I can do? :) Thanks so far.

@michael-ciniawsky
Copy link
Member

This seems to be currently failing on windows 🤔 https://ci.appveyor.com/project/sokra/webpack-dev-server/builds/19636987/job/0xps5athsnkc5m22

@donaldpipowitch
Copy link
Author

Is this a failure on my side? 🤔 Hard to tell for me. Should I rebaserebase with the current master?

@hiroppy
Copy link
Member

hiroppy commented Apr 29, 2019

@donaldpipowitch Could you rebase? If you don't rebase I will take over.

@donaldpipowitch
Copy link
Author

I tried to update it. If it doesn't work, I'd be happy if you could move on with the MR.

@hiroppy
Copy link
Member

hiroppy commented May 12, 2019

@donaldpipowitch Sorry but could you rebase again? We dropped travis and appveyor.

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

Successfully merging this pull request may close these issues.

4 participants