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

feat(server): warning for non web target (#2026) #2104

Closed

Conversation

Kristonitas
Copy link

  • 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?

Modified existing tests inside addEntries.test.js.

Motivation / Use-Case

Help new developers (who check terminal output) to more quickly detect potential misconfiguration when developing a web app while setting target: "node". A more detailed explanation of this: #2026 (comment). I acknowledge this is something very obvious once you know about the configuration.

Breaking Changes

If anyone is doing strict validation of webpack-dev-server output and using the configuration in question, this might affect them. It should be quite easy to filter/ignore or modify.

Additional Info

I made this minimal repo with two scripts to see the different behaviour with "node" and "web" targets: https://github.com/Kristonitas/live-reload-fail

@jsf-clabot
Copy link

jsf-clabot commented Jul 5, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #2104 (f093391) into master (8738bd1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2104      +/-   ##
==========================================
+ Coverage   94.55%   94.57%   +0.01%     
==========================================
  Files          32       32              
  Lines        1213     1217       +4     
  Branches      339      341       +2     
==========================================
+ Hits         1147     1151       +4     
  Misses         64       64              
  Partials        2        2              
Impacted Files Coverage Δ
lib/Server.js 97.15% <100.00%> (ø)
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 8738bd1...f093391. Read the comment docs.

@anikethsaha
Copy link
Member

@Kristonitas the CLA is not signed, please sign it.

if its already signed and still showing not signed, close the PR and reopen it !

@Kristonitas Kristonitas closed this Jul 6, 2019
@Kristonitas Kristonitas reopened this Jul 6, 2019
@Kristonitas
Copy link
Author

@evilebottnawi
Can you comment on this PR?
If you think I should take another approach or if this is not worth being added let me know.

if (config.liveReload !== false) {
log.error(`Live reload won't work with non web target`);
}
}
Copy link
Member

@alexander-akait alexander-akait Jul 10, 2019

Choose a reason for hiding this comment

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

Let's use info for this, warn may annoy developers

@alexander-akait
Copy link
Member

Close in favor #3094, sorry for delay 😞

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