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

Switch to webpack-dev-server (close #964) #975

Merged
merged 13 commits into from
Nov 8, 2017

Conversation

LinusBorg
Copy link
Contributor

@LinusBorg LinusBorg commented Oct 11, 2017

Why?

The reasons for this change are laid out in #964

What changed?

  • /config/dev-server.js and /config/dev-client.js are gone
  • webpack.dev.conf.js now returns a promise. This was necessary to keep the recently added portfinder feature working without writing a new dev-server.js file that wraps webpack-dev-server
  • That file now also has all the webpack-dev-server configuration, but most of the usually used options can be configured from /config/index.js
  • /test/e2e/runner.js was changed quite a bit to make it work with webpack-dev-server, but it works just fine.

What was added?

This PR also has a few goodies, all otpional changes are reachable through /config/index.js

  • The host&port are now shown in the console after each build with a nice message
    bildschirmfoto 2017-10-12 um 00 02 25
  • build progress is shown in the console during each build in development (already worked in npm run build).
  • You can now optionally receive a notification from your OS when the build fails, this can be activated with the notifyOnErrors option:
    bildschirmfoto 2017-10-12 um 00 03 43
  • You can deactivate eslint-loader. This is useful if you already have eslint support in your editor/IDE and don't want the warnings /errors shown by webpack
  • If you do use eslint-loader, it usually shows its errors in the error overlay in the browser and halts the build. You can now opt out of this, so eslint errors & warnings are only shown in the console.
  • General restructuring of /config/index.js:
    • moving all dev options to the top since these are the ones you use first/more often
    • added more comments and links to relevant webpack docs where I deemed it useful.
    • and tried to group the options into more logical blocks

Testing this PR

@vuejs-templates/collaborators and anyone interested: Please take this for a test-ride with:

vue init webpack#feature/webpack-dev-server(#964)

Edit: Appearantly the branch name breaks vue-cli, so you will have to check out the branch locally and then run vue init . nameForTestDirectory to test it

And report any and all problems in this PR thread. Thanks!

@yyx990803
Copy link
Contributor

Looks good to me on first glance - good to merge with one additional person testing it locally to make sure it works as intended.

@grakic
Copy link

grakic commented Oct 12, 2017

Package "node-notifier" is missing in the list of dependencies in generated packages.json and it is now used in utils.js

@LinusBorg
Copy link
Contributor Author

Thanks. Weird, no idea how that got lost but it's easy to fix :)

@LinusBorg
Copy link
Contributor Author

@grakic I just checked, it's not missing?

bildschirmfoto 2017-10-13 um 10 13 13

But anyway, I realized that I fixed a specific version which is not necessary, so I will push a small fix.

if (severity !== 'error') {
return
}
console.log('it should notify!!')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to remove this log 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops!

@@ -70,6 +67,7 @@
"karma-spec-reporter": "0.0.31",
"karma-webpack": "^2.0.2",
"mocha": "^3.2.0",
"node-notifier": "^5.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move this line outside of unit so it always gets installed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. thanks!

LinusBorg referenced this pull request Oct 13, 2017
This file is not transpiled, so we should stick to ES5
@LinusBorg LinusBorg self-assigned this Oct 16, 2017
@@ -5,7 +5,7 @@
"author": "{{ author }}",
"private": true,
"scripts": {
"dev": "node build/dev-server.js",
"dev": "webpack-dev-server --inline --hot --progress --config build/webpack.dev.conf.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to remove line 38 and 39 from build/webpack.dev.conf.js

    // https://github.com/glenjamin/webpack-hot-middleware#installation--usage
    new webpack.HotModuleReplacementPlugin(),

The --hot is the only one needed

// Add FriendlyErrorsPlugin
devWebpackConfig.plugins.push(new FriendlyErrorsPlugin({
compilationSuccessInfo: {
messages: [`You application is running here http://${config.dev.host}:${port}`],

Choose a reason for hiding this comment

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

s/You/Your/

@LinusBorg LinusBorg added this to the 1.2.0 milestone Nov 1, 2017
Copy link
Contributor

@posva posva left a comment

Choose a reason for hiding this comment

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

💯

@Tirke
Copy link
Contributor

Tirke commented Nov 9, 2017

I don't know what is going on, but I wanted to try the dev branch with the switch to webpack-dev-server and I had an issue right out of the box. npm run dev was complaining about a missing dependency :

These dependencies were not found:

* !!vue-style-loader!css-loader?{"minimize":false,"sourceMap":false}!../node_modules/vue-loader/lib/style-compiler/index?{"vue":true,"id":"data-v-7ba5bd90","scoped":false,"hasInlineConfig":false}!postcss-loader!../node_modules/vue-loader/lib/selector?type=styles&index=0&bustCache!./App.vue in ./src/App.vue
* !!vue-style-loader!css-loader?{"minimize":false,"sourceMap":false}!../../node_modules/vue-loader/lib/style-compiler/index?{"vue":true,"id":"data-v-469af010","scoped":true,"hasInlineConfig":false}!postcss-loader!../../node_modules/vue-loader/lib/selector?type=styles&index=0&bustCache!./HelloWorld.vue in ./src/components/HelloWorld.vue

To install them, you can run: npm install --save !!vue-style-loader!css-loader?{"minimize":false,"sourceMap":false}!../node_modules/vue-loader/lib/style-compiler/index?{"vue":true,"id":"data-v-7ba5bd90","scoped":false,"hasInlineConfig":false}!postcss-loader!../node_modules/vue-loader/lib/selector?type=styles&index=0&bustCache!./App.vue !!vue-style-loader!css-loader?{"minimize":false,"sourceMap":false}!../../node_modules/vue-loader/lib/style-compiler/index?{"vue":true,"id":"data-v-469af010","scoped":true,"hasInlineConfig":false}!postcss-loader!../../node_modules/vue-loader/lib/selector?type=styles&index=0&bustCache!./HelloWorld.vue

I added postcss-loader because it's not in package.json : npm install postcss-loader --save-dev

After that the dev server is up and running but with annoying warnings :

PostCSS Loader

Previous source map found, but options.sourceMap isn't set.
In this case the loader will discard the source map entirely for performance reasons.
See https://github.com/postcss/postcss-loader#sourcemap for more

Maybe I wasn't suppose to add postcss-loader ?

@LinusBorg
Copy link
Contributor Author

Hey, thanks for reporting. That may very well. Be the case, it's the dev branch. Some PRe that I merged probably need some refinement, I plan to look into this at the weekend

@LinusBorg
Copy link
Contributor Author

@Tirke postcss-loader was just added via #1038 and I opened a small PR to fix the sourcemap situation #1039

I can't test the PR right now, but you might take it for a spin if you want to :)

@Frondor
Copy link

Frondor commented Nov 14, 2017

To be honest, this is quite annoying:

It messes up my own debugging workflow with logs. Is there any way to disable those messages? @LinusBorg

@LinusBorg
Copy link
Contributor Author

LinusBorg commented Nov 14, 2017

I think clientLogLevel should be the setting you are looking for.

I probably should set this to something more reasonable, thanks for the pointer.

@Frondor
Copy link

Frondor commented Nov 14, 2017

Switched it to "error" and worked just fine!

@LinusBorg LinusBorg deleted the feature/webpack-dev-server(#964) branch December 12, 2017 15:18
frandiox pushed a commit to OnsenUI/vue-cordova-webpack that referenced this pull request Dec 25, 2017
…ates#975)

* Finished testable version.

* close vuejs-templates#960

* fix node-notifier version

* remove console.log

* moved general dependency out of unit test-only dependency block.

* fix typo

* ignore /test folder

* make HMR work correctly.

* improve console messages for HMR - now show filenames of replaced modules.

* fix typo in eslint-loader config

* move imports for the env files from /config/index.js directly into the webpack config.

Reasoning: thosen file imports are not configuration, so they don't belong inside of config/index.js

* fix wrong overlay config
shenron pushed a commit to shenron/webpack that referenced this pull request Mar 20, 2018
…ates#975)

* Finished testable version.

* close vuejs-templates#960

* fix node-notifier version

* remove console.log

* moved general dependency out of unit test-only dependency block.

* fix typo

* ignore /test folder

* make HMR work correctly.

* improve console messages for HMR - now show filenames of replaced modules.

* fix typo in eslint-loader config

* move imports for the env files from /config/index.js directly into the webpack config.

Reasoning: thosen file imports are not configuration, so they don't belong inside of config/index.js

* fix wrong overlay config
mednabouli pushed a commit to mednabouli/v-chacheli that referenced this pull request May 20, 2018
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