fix multi target

[ljqx]

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	no
Deprecations?	no
Tests added?	no
Docs updated?	no
Fixed tickets	none
License	MIT

This is a bug when running grunt webpack with multi targets. If we define multi targets for this plugin, then only 1 or 2 of them will run when user calls grunt webpack, and the tasks are picked randomly.

This is because this task is defined as a grunt basic task, but actually it is designed to be used as a grunt multi tasks. And when we call grunt webpack it should handle all targets, but now the code use a global done for all targets, any of them finished will cause this task done.

The test fail because of Grunt bug, I'll work on that fix first.

An example which repro the bug:

/* eslint no-process-env: 1, camelcase: 1 */
const path            = require('path');
const _               = require('underscore');
const webpack         = require('webpack');

module.exports = function (grunt) {
  function webpackConfig(dependency) {
    return {
      entry: require.resolve(dependency),
      output: {
        filename: `${dependency}.js`,
        path: path.join(__dirname, 'dist'),
        libraryTarget: 'amd',
      },
      module: {
        rules: [{
          test: () => true,
          use: 'imports-loader?define=>false',
        }],
      },
      externals: [
        ..._.keys(require(`${dependency}/package.json`).dependencies),
        'bluebird',
      ],
      devtool: 'inline-source-map',
      plugins: [
        new webpack.ProvidePlugin({
          Promise: 'bluebird',
        }),
      ],
    };
  }

  grunt.initConfig({
    webpack: {
      cldrjs: webpackConfig('cldrjs'),
      globalize: webpackConfig('globalize'),
      html2canvas: webpackConfig('html2canvas'),
    },
  });

  grunt.loadNpmTasks('grunt-webpack');

  grunt.registerTask('default', [
    'webpack',
  ]);
};

[jsf-clabot]

All committers have signed the CLA.

[danez]

It is on purpose defined as Basic task as we use custom configurations which grunt cannot handle when using multitask. (#122)
But we need to fix that it is not running all targets

[codecov]

Codecov Report

Merging #140 into master will increase coverage by 0.97%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   67.67%   68.65%   +0.97%     
==========================================
  Files           8        8              
  Lines         198      201       +3     
  Branches       50       51       +1     
==========================================
+ Hits          134      138       +4     
  Misses         52       52              
+ Partials       12       11       -1

Impacted Files	Coverage Δ
tasks/webpack.js	78.04% <100%> (+4.36%)	⬆️

---

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 6f7ed27...69a4e38. Read the comment docs.

[ljqx]

@danez I've changed back to a basic task, thanks for the notice

[ljqx]

@danez , weird that I've already signed the CLA agreement but it shows not here. Did I do something wrong? And why you prefer not add peerDependency grunt? it seems cleaner to me to know which versions of grunt this plugin supports..

[ljqx]

ok, it's the email address, I'll fix it.

[ljqx]

squashed to force update :) the license check passes now

[danez]

This will have weird effects if different targets have different keepalive set. Can we instead maybe check every target and if at least one target has keepalive===true then do it?
So basically outside the forEach have

let keepalive = false;

and then for every target:

keepalive = keepalive || opts.keepalive;

Als the counter decrease has to be outside this if, as otherwise we can potentially never reach 0 right now if on target has keepalive true.

so maybe just simplify to

if (--runningTargetCount === 0 && !keepalive) done();

[ljqx]

Fixed, thanks

[danez]

Looks good besides the comment I made.

Would you be willing doing the same thing then also for webpack-dev-server? If not it's okay, I can easily duplicate.

[ljqx]

@danez , OK, I'll do this in another PR.

[danez]

Thank you