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

Gulp update #1696

Merged
merged 10 commits into from Jan 19, 2017
Merged

Gulp update #1696

merged 10 commits into from Jan 19, 2017

Conversation

pashist
Copy link
Contributor

@pashist pashist commented Oct 28, 2016

  • Removed webpack task. Instead used webpack-dev-middleware. It enabled nice hot module replacement feature of webpack. Also hope it reduced rebuilding time.
  • Removed less and sass tasks. Instead used less-loader and sass-loader for webpack with sourcemaps. Now .less and .scss files can be imported in packages.
  • Fixed page reloading in nodemon.

@liorkesos
Copy link
Member

Nice!
+1

On Oct 28, 2016 18:45, "pashist" notifications@github.com wrote:

  • Removed webpack task. Instead used webpack-dev-middleware. It
    enabled nice hot module replacement
    http://webpack.github.io/docs/hot-module-replacement feature of
    webpack. Also hope it reduced rebuilding time.
  • Removed less and sass tasks. Instead used less-loader and sass-loader
    for webpack with sourcemaps. Now .less and .scss files can be imported
    in packages.
  • Fixed page reloading in nodemon.

You can view, comment on, or merge this pull request online at:

#1696
Commit Summary

  • gulp update
  • Merge branch 'master' of git://github.com/linnovate/mean into
    gulp_update
  • updated dependencies
  • updated development.js
  • updated development.js
  • updated development.js
  • moved webpack to middleware

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1696, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAO9Iwkk3Izukq4Z59nEZTtNhXCLdRNbks5q4hiKgaJpZM4KjlC8
.

@timelf123
Copy link
Contributor

This is awesome, nice work. Testing now

@jwebbdev
Copy link
Member

@pashist With these changes, what are the paths of execution when js/css change? My concern is that there's a webpack-dev path going off triggered by changes and a nodemon task executing from the same changes, racing towards a livereload which may happen before or after webpack is built (and if I'm reading the webpack-dev stuff the nodemon restart isn't actually necessary in this case).

@pashist
Copy link
Contributor Author

pashist commented Oct 28, 2016

@rjVapes basically files handled by webpack should not intersect with nodemon watched files, in nodemon config they are added to ignore section:

ignore: [
        'node_modules/',
        'bower_components/',
        'bundle/',
        '../app.js',                           // handled by webpack
        'logs/',
        'packages/**/public/',   // <--- here handled by webpack too
        '.DS_Store', '**/.DS_Store',
        '.bower-*',
        '**/.bower-*',
        '**/tests'
      ],

Even if same file will watched by webpack and nodemon, after changes triggered, nodemon will restart server.js with webpack-dev-middleware inside, so any webpack building process will interrupt and executed again after server.js start

Copy link
Contributor

@timelf123 timelf123 left a comment

Choose a reason for hiding this comment

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

Great work. Saving a frontend file in the past would sometimes require a manual call to webpack or restarting gulp or resaving the file due to the tomfoolery that rjvapes described with nodemon/livereload/webpack/gulp.

No race conditions, smart middleware, 75% faster bundle times and HMR support for those that dare go down that path -- wins all around

@liorkesos
Copy link
Member

Great job Pavel.
Lior

On Oct 28, 2016 21:42, "Tim Elfelt" notifications@github.com wrote:

@timelf123 approved this pull request.

Great work. Saving a frontend file in the past would sometimes require a
manual call to webpack or restarting gulp due to the tomfoolery that
rjvapes described with nodemon/livereload/webpack/gulp.

No race conditions, smart middleware, 75% faster bundle times and HMR
support for those that dare go down that path


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1696 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAO9I3ZdB8lCXCQIx8xCQcv3WtmsWKplks5q4kIggaJpZM4KjlC8
.

@timelf123
Copy link
Contributor

One minor fix webpack.test.js to make SASS and svg etc work:

'use strict'

module.exports = {
  module: {
    loaders: [{
      test: /\.css$/,
      loader: 'style-loader!css-loader'
    },{
      test: /\.scss$/,
      loaders: ['style', 'css?sourceMap', 'sass?sourceMap']
    }, {
      test: /\.js$/,
      exclude: /(node_modules|bower_components|lib)/,
      loader: 'babel?presets[]=es2015&presets[]=stage-1'
    }, {
      test: /\.(png|woff|woff2|eot|ttf|svg)(\?v=[0-9]\.[0-9]\.[0-9])?$/,
      loader: 'url'
    }]
  },
  resolve: {
    modulesDirectories: ['bower_components', 'node_modules']
  }
}

Other than that this looks good and I merged to my fork already, thanks again

@jwebbdev
Copy link
Member

jwebbdev commented Oct 29, 2016

@pashist Ah, okay I didn't notice the ignore of packages/**/public. Does webpack-dev trigger a page reload on its own? I know I saw it was capable of it, but didn't notice any config changes to set it up explicitly.

Also, since you're editing the gulp, @dalenguyen in the gitter chat recently mentioned that server side html files need a nodemon restart (I guess express caches them and I didn't realize) so we probably need to feed the /packages/**/server/ htmls to nodemon, and the /packages/**/public/ htmls to livereload.

added `--no-wdm` arg for disabling webpack dev middleware and using webpack task instead
updated webpack test config linnovate#1696 (comment)
@pashist
Copy link
Contributor Author

pashist commented Oct 29, 2016

Regarding 26962b9. Since webpack-dev-middleware is an express server middleware, it will recompile bundle every time when nodemon restarts server due to server files changes. It's no problem if we currently develop public part. But if it's server part development, recompiling public files is unnecessary and --no-wdm option allow avoid this. With --no-wdm option public files changes will be tracked and bundle will rebuilded, just without HMR support. Webpack task used webpack-stream and watch mode, so recompiling time should be ok.
How you think is this useful ? And perhaps we should use this option as default ?

@rjVapes perhaps we could just disable files .html files caching in dev env ?

@timelf123
Copy link
Contributor

timelf123 commented Oct 29, 2016

@pashist So the issue is when nodemon restarts the process containing the webpack middleware, it loses the bundle information stored in memory, right?

Two workarounds are either use a separate process for webpack middleware, or find a way to reload your server routes without restarting the whole process (this is maybe for next milestone?, essentially use webpack to generate server bundle and require(package.routes) allowing server side HMR)

I'm good with a --no-wdm option as a fallback/default for now. Also webpack --watch is usually faster for me anyway and the only thing we can currently HMR anyway is stylesheets

@adskjohn
Copy link
Contributor

@pashist Sounds like a good solution (disabling .html cache). Any time lost from not having the small volume of server side html cached I'm sure would more than be made up by not having to reload the server.

webpack dev middleware available with `--wdm` option
@timelf123
Copy link
Contributor

@pashist thoughts on rebasing and merging?

@timelf123
Copy link
Contributor

@pashist @liorkesos can we get this merged?

@liorkesos liorkesos merged commit f08fc2c into linnovate:master Jan 19, 2017
loyaldev03 added a commit to loyaldev03/mean_stack_dev that referenced this pull request Jan 8, 2018
added `--no-wdm` arg for disabling webpack dev middleware and using webpack task instead
updated webpack test config linnovate/mean#1696 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MEAN.io RC1
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants