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

set default mode to development #1562

Closed
wants to merge 7 commits into from
Closed

set default mode to development #1562

wants to merge 7 commits into from

Conversation

andreasnilssondev
Copy link

@andreasnilssondev andreasnilssondev commented Nov 7, 2018

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

If this needs any tests I would appreciate some help what type of test, and where to add it

Motivation / Use-Case

Set default mode to development. webpack config and CLI option will override the default if provided.
See #1560 and #1327

Breaking Changes

Additional Info

As far as I could tell the devServer options (just below the line changed in this PR) are only picked up from the first array item if the webpack config is an array(?). So I used the same principle for detecting mode in the config.

@jsf-clabot
Copy link

jsf-clabot commented Nov 7, 2018

CLA assistant check
All committers have signed the CLA.

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.

Looks good!

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.

Can we add tests?

Copy link
Member

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

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

While the code is technically correct, I'm not sure this is the right thing to do.

webpack defaults mode on production. I think WDS should do the same. I understand that WDS is a development tool but we should teach our users to specify a mode, not to use the default one. Moreover, webpack raises a warning about mode not being specified and being defaulted to production. With this change, the warning vanishes which is not correct in my opinion.

@andreasnilssondev
Copy link
Author

andreasnilssondev commented Nov 8, 2018

Hi @ooflorent

Personally I think what Webpack's warning is really saying (the reason behind it) is "Hey, I don't know if you want production or development, please specify or the result may be really wrong". That makes sense in Webpack's case, because from running webpack there's no way to tell if the user wants production or development.

But in this case, from webpack-dev-server, we do know that the user wants (or should have) development, so I don't see the reason to ask for it. (I guess I see webpack-dev-server as the "webpack user" that sets the mode)

Webpack will still warn when the user wants to build a production build with the webpack command

I could definitely be missing something though, maybe you're thinking of a specific scenario?

EDIT: to be clear it's not my decision in the end, I have no power here hehe, I just added my view on it.
I will look at tests meanwhile

@alexander-akait
Copy link
Member

@nAndreas

EDIT: to be clear it's not my decision in the end, I have no power here hehe, I just added my view on it.
I will look at tests meanwhile

We need feedback from other developers

@MatTheCat
Copy link

For now there are 78 👍 on #1327

With this change, the warning vanishes which is not correct in my opinion.

The warning will appear when webpack is used.

I heard a lot about “zero configuration” and “sensible defaults”. Is it not sensible for webpack-dev-server to imply a dev enviroment?

@odinho
Copy link
Contributor

odinho commented Dec 4, 2018

IMHO this sounds like a sane default (so one extra random developer there).

@nAndreas: If you rebase, Travis should re-run and hopefully not fail.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #1562 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
+ Coverage   74.34%   74.52%   +0.18%     
==========================================
  Files          10       10              
  Lines         690      691       +1     
==========================================
+ Hits          513      515       +2     
+ Misses        177      176       -1
Impacted Files Coverage Δ
bin/webpack-dev-server.js 57.06% <100%> (+0.23%) ⬆️
lib/Server.js 81.53% <0%> (+0.26%) ⬆️

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 4bf1f76...d1501b2. Read the comment docs.

@alexander-akait
Copy link
Member

@nAndreas we need add tests and we can merge this

@andreasnilssondev
Copy link
Author

Hi,
I will not have access to a laptop in december, if anyone else feel like taking over this PR, feel free. If nobody worked on it when I am back around new years, I can look at it.
Thanks

@andreasnilssondev
Copy link
Author

Hi,
I've started looking at the tests and how to effectively test this change, but I'm lost when looking at other tests and don't have the time to deep dive into it. I will have to leave this one as it is now sorry.

Feel free to take this PR anyone who wants to write the tests.

Thanks

@alexander-akait
Copy link
Member

Done in master

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.

7 participants