apache / cordova-paramedic

Apache Cordova - Paramedic
https://cordova.apache.org/
Apache License 2.0
36 stars 53 forks source link

Refactor ParamedicConfig #169

Open janpio opened 5 years ago

janpio commented 5 years ago

Currently the paramedic config is built via: https://github.com/apache/cordova-paramedic/blob/5b8d30613d0fcb7d9a9abf16dd69783e5932d5c9/main.js#L78-L206 and https://github.com/apache/cordova-paramedic/blob/5b8d30613d0fcb7d9a9abf16dd69783e5932d5c9/lib/ParamedicConfig.js#L280-L327

This is probably buggy, as if there is a config file, the defaults from parseFromArguments are not applied. Also the manual setting of config values in main.js is missing some parameters etc.

A better way would be to replace everything in main.js with e.g. var paramedicConfig = new ParamedicConfig(argv, pathToParamedicConfig); and then do all the logic in the constructor: Read and apply the file if supplied, then merge over all the manual arguments that are present in argv.

Pseudocode from @erisu:

constructor (args, file) {
        // defaults
        this._config = {
            add: 'my',
            required: 'default',
            values: 'here'
        };

        // apply file next
        this._config = Object.assign(this._config, file || {});

        // then args
        this._config = Object.assign(this._config, args || {});
    }
erisu commented 5 years ago

I believe all the configs are 1-level depth.

If there are any cases where it is not, Object.assign wont be able to handle this. You will need to do a deep merge.

Here is sample code I use in cordova-electron to handle deep merging.

https://github.com/apache/cordova-electron/blob/de08c6b50d6d11a002984abb1438991c648f2737/bin/templates/cordova/lib/util.js#L20-L32