Skip to content
This repository has been archived by the owner on Feb 7, 2022. It is now read-only.

support config as export function v2 #48

Merged
merged 5 commits into from
Jul 28, 2017
Merged

Conversation

Robbilie
Copy link
Contributor

#35

based on the changes suggested in that PR but apparently not further evaluated, this change should do what is required, even without all the steps in between mentioned here https://github.com/trivago/parallel-webpack/pull/35/files/bbdc8ffa14a2aa81adcdf5e1b5a5f1b951ffd215#r108773189

process.argv is in several locations overwritten by the args passed to the initial run.js and therefore always contain the required args so we just need to filter them, adjust them to kv pairs and then reduce them to an object to be passed as the env object

trivago#35

based on the changes suggested in that PR but apparently not further evaluated, this change should do what is required, even without all the steps in between mentioned here https://github.com/trivago/parallel-webpack/pull/35/files/bbdc8ffa14a2aa81adcdf5e1b5a5f1b951ffd215#r108773189

process.argv is in several locations overwritten by the args passed to the initial run.js and therefore always contain the required args so we just need to filter them, adjust them to kv pairs and then reduce them to an object to be passed as the env object
@Robbilie
Copy link
Contributor Author

@pago could you check this out please? :)

@pago
Copy link
Contributor

pago commented Jul 25, 2017

Very elegant solution but is it sufficient? Most command line tools accept varying syntaxes such as --env.foo bar, which would be equivalent to --env.foo=bar and then there's of course also --env.foo "bar" and "--env.foo="bar"`.

Might be safer to parse the argv again using minimist and to iterate over its keys, picking the key-value pars that start with env.?

@Robbilie
Copy link
Contributor Author

i did not think about that one, let me try this real quick ;)

@Robbilie
Copy link
Contributor Author

better? :D

@pago
Copy link
Contributor

pago commented Jul 27, 2017

Looks good now. I'd like to give it a quick test run before merging.

@evisong
Copy link

evisong commented Jul 27, 2017

Hi,

I'm using ES6 in webpack.config.babel.js, seems parallel-webpack can parse ES6 but I need to do the following workaround in loadConfigurationFile.js to read the config from default export:

    else if (typeof config === 'object' && config.default) {
        return config.default(require('minimist')(process.argv, { '--': true }).env || {});
    }

@Robbilie
Copy link
Contributor Author

@evisong i think my change should do?

@Robbilie
Copy link
Contributor Author

@pago if you feel lazy, clone this and test a basic config for the function support:
https://github.com/Robbilie/parallel-webpack-test

@anilanar
Copy link
Contributor

anilanar commented Jul 27, 2017

What do you mean exactly? parallel-webpack can parse ES6 probably because you don't have any import statements and you have a recent node version that supports all ES6 features that you are using in your config.

At trivago, we write webpack configs in ES6 but there's always a bootstrapping code that does the following:

// example webpack.config.js
module.exports = require('./path/to/real/config.js').default;

However, if webpack is capable of handling exports.default, then parallel-webpack should do the same. However we should check if exports.__esModule is true before doing it.

var config = require(configPath);
if (typeof config === 'function')
return config(require('minimist')(process.argv, { '--': true }).env || {});
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Unnecessary code duplication.
  2. Wrong code. There are 4 different permutations for function/object and default/no default.
  3. Split this function. It's too long already.

It should be refactored along these lines:

var configModule = require(configPath);
var configDefault = configModule && configModule.__esModule ? configModule.default : configModule;

var config = typeof configDefault === 'function' ? callConfigFunction(configDefault) : configDefault;
return config;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

babel sets the __esModule i guess?
ill adjust it a bit in a moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took me a bit to remember that the es module export can be an object too ^^
not sure if you like the way i split the methods

@pago pago merged commit 6f45fce into trivago:master Jul 28, 2017
@pago
Copy link
Contributor

pago commented Jul 28, 2017

Thanks @Robbilie ! I've added a test case (now that we can do that). Works nicely. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants