trivago / parallel-webpack

Builds multi-config webpack projects in parallel
BSD 3-Clause "New" or "Revised" License
1.48k stars 96 forks source link

Handle config function return promise of config object array #84

Closed chenesan closed 5 years ago

chenesan commented 6 years ago

For #76 . I've changed a little how startFarm and webpackWorker get config and added a test for config function return promise of config object array. After the change, there are some tests get failed. And I'm not sure how to handle them. @pago @efegurkan I need some advices about them:

webpackWorker.spec.js

This two all expect Promise.reject will be called when call webpackWorker function. Before this change they will be called immediately. But since we have to get the config array after promise resolved, I move the config length checking part into the then part, so they will not be called immediately and we cannot check the mock synchronously. I have tried checking calls asynchronously in test (webpackWorker.then(...)) but it seems that webpackWorker will not finish until jest test time out.

index.spec.js

The above three snapshot tests will lose one line "[WEBPACK] Building 1 target" since we have to move the console.log into promise(again now we have to resolve config function promise before we know its length). Not sure if it's ok to pass those snapshot tests.

paales commented 5 years ago

@chenesan Any updates on this?

chenesan commented 5 years ago

I may try to take a look again this week.

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-3.4%) to 83.524% when pulling 8d7df7b414ad82644ab71dc9483eb864ae10bb11 on chenesan:resolve-config-func-return-promise-of-array into 66da53e44a04795ab8ea705c28f9848ae5e7c7d7 on trivago:master.

chenesan commented 5 years ago

@pago I just fixed the tests and it's ok to review it :-) You could also test it by manually create a minimal config file which is a function return promise of config array and run ./bin/run:

/* webpack.config.js */
var path = require('path');
module.exports = () => Promise.resolve([{
    entry: './pageA.js',
    output: {
        path: path.resolve(__dirname, './dist'),
        filename: 'pageA.bundle.js'
    }
}, {
    entry: './pageB.js',
    output: {
        path: path.resolve(__dirname, './dist'),
        filename: 'pageB.bundle.js'
    }
}]);
pago commented 5 years ago

Looks very good to me. Thank you for the time you put into building this feature!

@byara could you please also take a look? Then we can proceed with merging and releasing this feature. :)

byara commented 5 years ago

Thank you @chenesan, good work 👍 @pago It looks good to me :)

chenesan commented 5 years ago

Hi @pago and @byara sorry for pinging, anything I could do to help proceeding?

pago commented 5 years ago

Hi @chenesan , I'm sorry I didn't manage to take care of this before my vacation. I'll release it in a moment.

pago commented 5 years ago

Released as 2.4.0