From 78c036ca7fc3ce3099f956684d46ac8f2e76bf74 Mon Sep 17 00:00:00 2001 From: David Eglin Date: Tue, 22 Nov 2016 18:35:27 +0000 Subject: [PATCH 1/4] replace object.assign merging of AppJs opts and argument opts to allow nested opts to merge properly --- lib/config.js | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/config.js b/lib/config.js index 08c8825..9ea5d10 100644 --- a/lib/config.js +++ b/lib/config.js @@ -26,7 +26,8 @@ module.exports = class Config { throw new Error('[spike constructor] option "root" is required') } // merges API options into app.js options - let allOpts = Object.assign(this.parseAppJs(opts), opts) + let appJsOpts = this.parseAppJs(opts) + let allOpts = mergeNestedObjects(appJsOpts, opts) this.transformSpikeOptionsToWebpack(this.validateOpts(allOpts)) this.project = project } @@ -284,3 +285,31 @@ function filterKeys (obj, keys) { for (const k in obj) { if (keys.indexOf(k) > 0) res[k] = obj[k] } return res } + +function mergeProperties(propertyKey, firstObject, secondObject) { + var propertyValue = firstObject[propertyKey]; + + if (secondObject === undefined || secondObject[propertyKey] === undefined) { + return firstObject[propertyKey]; + } else if (typeof(propertyValue) === "object") { + return mergeNestedObjects(firstObject[propertyKey], secondObject[propertyKey]); + } + + return secondObject[propertyKey]; +} + +function mergeNestedObjects(firstObject, secondObject) { + var finalObject = {}; + + // Merge first object and its properties. + for (var propertyKey in firstObject) { + finalObject[propertyKey] = mergeProperties(propertyKey, firstObject, secondObject); + } + + // Merge second object and its properties. + for (var propertyKey in secondObject) { + finalObject[propertyKey] = mergeProperties(propertyKey, secondObject, firstObject); + } + + return finalObject; +} From 1ef02217d66baa47aece18ebf2387be10790eabf Mon Sep 17 00:00:00 2001 From: David Eglin Date: Tue, 22 Nov 2016 22:08:09 +0000 Subject: [PATCH 2/4] fix code formatting for lint and add test for new opts merge --- lib/config.js | 40 ++++++++++++++++++++-------------------- test/app_config.js | 7 +++++++ 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/lib/config.js b/lib/config.js index 9ea5d10..557d06f 100644 --- a/lib/config.js +++ b/lib/config.js @@ -286,30 +286,30 @@ function filterKeys (obj, keys) { return res } -function mergeProperties(propertyKey, firstObject, secondObject) { - var propertyValue = firstObject[propertyKey]; +function mergeProperties (propertyKey, firstObject, secondObject) { + var propertyValue = firstObject[propertyKey] - if (secondObject === undefined || secondObject[propertyKey] === undefined) { - return firstObject[propertyKey]; - } else if (typeof(propertyValue) === "object") { - return mergeNestedObjects(firstObject[propertyKey], secondObject[propertyKey]); - } + if (secondObject === undefined || secondObject[propertyKey] === undefined) { + return firstObject[propertyKey] + } else if (typeof propertyValue === 'object') { + return mergeNestedObjects(firstObject[propertyKey], secondObject[propertyKey]) + } - return secondObject[propertyKey]; + return secondObject[propertyKey] } -function mergeNestedObjects(firstObject, secondObject) { - var finalObject = {}; +function mergeNestedObjects (firstObject, secondObject) { + var finalObject = {} - // Merge first object and its properties. - for (var propertyKey in firstObject) { - finalObject[propertyKey] = mergeProperties(propertyKey, firstObject, secondObject); - } + // Merge first object and its properties. + for (var propertyKey in firstObject) { + finalObject[propertyKey] = mergeProperties(propertyKey, firstObject, secondObject) + } - // Merge second object and its properties. - for (var propertyKey in secondObject) { - finalObject[propertyKey] = mergeProperties(propertyKey, secondObject, firstObject); - } + // Merge second object and its properties. + for (var propertyKey2 in secondObject) { + finalObject[propertyKey2] = mergeProperties(propertyKey2, secondObject, firstObject) + } - return finalObject; -} + return finalObject +} diff --git a/test/app_config.js b/test/app_config.js index 91bb84c..5c6d141 100644 --- a/test/app_config.js +++ b/test/app_config.js @@ -13,6 +13,13 @@ test('API config overrides app.js config', (t) => { }) }) +test('API config merges properly with app.js config', (t) => { + return compileFixture(t, 'app_config', { testing: { bar: 'double override' } }).then(({res}) => { + t.truthy(res.stats.compilation.options.testing.foo === 'override') + t.truthy(res.stats.compilation.options.testing.bar === 'double override') + }) +}) + test('throws error for invalid app.js syntax', (t) => { return t.throws(() => compileFixture(t, 'app_config_error'), /Error: wow/) }) From 155f81c115496c2d1876c06fb4914ae936168fa3 Mon Sep 17 00:00:00 2001 From: David Eglin Date: Tue, 22 Nov 2016 22:54:25 +0000 Subject: [PATCH 3/4] use _.merge instead of custom functions. Improve merge test. --- lib/config.js | 30 +----------------------------- test/app_config.js | 2 +- test/fixtures/app_config/app.js | 4 +++- 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/lib/config.js b/lib/config.js index 557d06f..84e7072 100644 --- a/lib/config.js +++ b/lib/config.js @@ -27,7 +27,7 @@ module.exports = class Config { } // merges API options into app.js options let appJsOpts = this.parseAppJs(opts) - let allOpts = mergeNestedObjects(appJsOpts, opts) + let allOpts = merge(appJsOpts, opts) this.transformSpikeOptionsToWebpack(this.validateOpts(allOpts)) this.project = project } @@ -285,31 +285,3 @@ function filterKeys (obj, keys) { for (const k in obj) { if (keys.indexOf(k) > 0) res[k] = obj[k] } return res } - -function mergeProperties (propertyKey, firstObject, secondObject) { - var propertyValue = firstObject[propertyKey] - - if (secondObject === undefined || secondObject[propertyKey] === undefined) { - return firstObject[propertyKey] - } else if (typeof propertyValue === 'object') { - return mergeNestedObjects(firstObject[propertyKey], secondObject[propertyKey]) - } - - return secondObject[propertyKey] -} - -function mergeNestedObjects (firstObject, secondObject) { - var finalObject = {} - - // Merge first object and its properties. - for (var propertyKey in firstObject) { - finalObject[propertyKey] = mergeProperties(propertyKey, firstObject, secondObject) - } - - // Merge second object and its properties. - for (var propertyKey2 in secondObject) { - finalObject[propertyKey2] = mergeProperties(propertyKey2, secondObject, firstObject) - } - - return finalObject -} diff --git a/test/app_config.js b/test/app_config.js index 5c6d141..b108a29 100644 --- a/test/app_config.js +++ b/test/app_config.js @@ -15,7 +15,7 @@ test('API config overrides app.js config', (t) => { test('API config merges properly with app.js config', (t) => { return compileFixture(t, 'app_config', { testing: { bar: 'double override' } }).then(({res}) => { - t.truthy(res.stats.compilation.options.testing.foo === 'override') + t.truthy(res.stats.compilation.options.testing.baz === 'override') t.truthy(res.stats.compilation.options.testing.bar === 'double override') }) }) diff --git a/test/fixtures/app_config/app.js b/test/fixtures/app_config/app.js index 71d4551..4ce9b66 100644 --- a/test/fixtures/app_config/app.js +++ b/test/fixtures/app_config/app.js @@ -1,5 +1,7 @@ module.exports = { testing: { - foo: 'override' + foo: 'override', + bar: 'override', + baz: 'override' } } From 13bfb5dfa8e275f021269cc7e36dbc4076d7dcd9 Mon Sep 17 00:00:00 2001 From: David Eglin Date: Tue, 22 Nov 2016 23:10:33 +0000 Subject: [PATCH 4/4] put this.parseAppJs() back inline, rather than as its own variable --- lib/config.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/config.js b/lib/config.js index 84e7072..dc5b5d9 100644 --- a/lib/config.js +++ b/lib/config.js @@ -26,8 +26,7 @@ module.exports = class Config { throw new Error('[spike constructor] option "root" is required') } // merges API options into app.js options - let appJsOpts = this.parseAppJs(opts) - let allOpts = merge(appJsOpts, opts) + let allOpts = merge(this.parseAppJs(opts), opts) this.transformSpikeOptionsToWebpack(this.validateOpts(allOpts)) this.project = project }