Optimize JavaScript tasks

[conorbarclay]

Hey guys,

I've been using FP with a slightly modified configuration, similar to the proposed, which offers up some pretty significant optimizations:

-An exclusion added for "node_modules" to the Webpack config to avoid needless transforming on that folder during dev. Small speed bump here.

-JavaScript task broken down into separate build and watch functions. The purpose of this is two fold:

1. Being able to use Webpack's native watcher function (as opposed to watching JS with Gulp). During dev, JS updates are MUCH faster - basically instant.

2. Webpack's native source mapping, I've found, is markedly more useable and speedier than the maps generated by gulp-sourcemap.

The reason behind splitting out the JS watch from the main watch() function is so that they can be run in parallel. Otherwise, the Webpack watcher "blocks" changes to other assets (you can see this happen by adding {watch: true} to the webpackConfig object in a fresh FP install).

Give it a shot and let me know what you think!

[colin-marshall]

@conorbarclay thanks for doing this, I definitely see a difference in speed. I think we also want error handling. As recommended in shama/webpack-stream#34 (comment), I changed the return statement to:

return gulp.src(PATHS.entries)
  .pipe(named())
  .pipe(
    webpackStream(webpackConfig, webpack2, webpackChangeHandler)
    .on('error', (err) => {
      gutil.log('WEBPACK ERROR', err);
    })
  )
  .pipe(gulp.dest(PATHS.dist + '/assets/js'));

It breaks on syntax errors if the error is present prior to running npm start. If you start error free, it resumes the watch process and does not break if you introduce a syntax error. Is that something we can fix? If not, no big deal.

[conorbarclay]

@colin-marshall I think that would be acceptable...if anything, I appreciate my watch not being broken on errors during dev -- as long as I can see that there's an error, so I can fix it and then have the watch carry-on is ideal.

[colin-marshall]

@conorbarclay sounds good. Please add a commit with the error reporter from my previous post. Thanks!

[colin-marshall]

@conorbarclay the watch breaks on syntax errors in the commit you just added:

[10:51:00] webpack is watching for changes
[10:51:28] 'webpack:watch' errored after 30 s
[10:51:28] Error in plugin 'webpack-stream'
Message:
    ./src/assets/js/app.js
Module parse failed: /Users/colinmarshall/Projects/Forks/FoundationPress/src/assets/js/app.js Unexpected token (11:9)
You may need an appropriate loader to handle this file type.
| //import './lib/foundation-explicit-pieces';
|
| dsimport './lib/demosite';
|
| $(document).foundation();
 @ multi ./src/assets/js/app.js
Details:
    domain: [object Object]
    domainThrown: true
[10:51:28] 'default' errored after 31 s
[10:51:28] The following tasks did not complete: watch
[10:51:28] Did you forget to signal async completion?
>>> elapsed time 33s

Is there a reason you chose to do it differently than the way I suggested?

[conorbarclay]

My bad, will revise. Was trying to move the error logging to the callback function to keep things organized.

[colin-marshall]

No problem! Thanks @conorbarclay.

Sorry to be a pain but can you reset your branch to 7d6237e and then add the error handling? You'll have to force the changes with -f when you push them to your fork. My apologies for not stating this in my previous post.

As you can see in #1162 and #1123 (comment) we are trying to make commits more clear to our users and so we should also revert to make corrections like this instead of committing on top of them.

[olefredrik]

@colin-marshall @conorbarclay Do you consider this pull request ready for merge into master?

[conorbarclay]

@colin-marshall @olefredrik I'd say it's ready to fly!

[colin-marshall]

@olefredrik if you have tested and think it's ok, then go ahead and merge.