Skip to content

Commit 4a8c119

Browse files
refactor(module): simplify and fix configMerge (#4203)
I had a hard time understanding `configMerge` - the old `arguments` slicing and nested loops took me a while. The jsdoc didn't help much. After it finally clicked, I rewrote it to be readable: rest param + a simple `for...of` instead of the `while`-stack. And there's one real behavior fix. With this default and user config: ```js defaults: { foo: { a: 1 } } config: { foo: [10, 20] } ``` the old code merged them into `{ foo: { 0: 10, 1: 20, a: 1 } }`. Now the array just replaces the object -> `{ foo: [10, 20] }`. I added a small test for that case. Only affects modules using `configDeepMerge: true` that override an object default with an array, so the impact is tiny.
1 parent ed95826 commit 4a8c119

2 files changed

Lines changed: 36 additions & 39 deletions

File tree

js/module.js

Lines changed: 22 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -422,48 +422,31 @@ export class Module {
422422
globalThis.Module = Module;
423423

424424
/**
425-
* Merging MagicMirror² (or other) default/config script by `@bugsounet`
426-
* Merge 2 objects or/with array
427-
*
428-
* Usage:
429-
* -------
430-
* this.config = configMerge({}, this.defaults, this.config)
431-
* -------
432-
* arg1: initial object
433-
* arg2: config model
434-
* arg3: config to merge
435-
* -------
436-
* why using it ?
437-
* Object.assign() function don't to all job
438-
* it don't merge all thing in deep
439-
* -> object in object and array is not merging
440-
* -------
441-
*
442-
* Todo: idea of Mich determinate what do you want to merge or not
443-
* @param {object} result the initial object
444-
* @returns {object} the merged config
425+
* Deep-merge module defaults with the user config.
426+
* Used by Module.setConfig when configDeepMerge is enabled.
427+
* Nested plain objects are merged recursively.
428+
* All other values (strings, numbers, arrays, …) are overwritten.
429+
* Sources are applied left to right; later values win.
430+
* @param {object} target The object to merge into (mutated and returned).
431+
* @param {...object} sources Objects whose properties are merged into target.
432+
* @returns {object} The merged target object.
445433
*/
446-
function configMerge (result) {
447-
const stack = Array.prototype.slice.call(arguments, 1);
448-
let item, key;
449-
450-
while (stack.length) {
451-
item = stack.shift();
452-
for (key in item) {
453-
if (item.hasOwnProperty(key)) {
454-
if (typeof result[key] === "object" && result[key] && Object.prototype.toString.call(result[key]) !== "[object Array]") {
455-
if (typeof item[key] === "object" && item[key] !== null) {
456-
result[key] = configMerge({}, result[key], item[key]);
457-
} else {
458-
result[key] = item[key];
459-
}
460-
} else {
461-
result[key] = item[key];
462-
}
463-
}
434+
function configMerge (target, ...sources) {
435+
const isPlainObject = (value) => value?.constructor === Object;
436+
437+
for (const source of sources) {
438+
for (const [key, sourceValue] of Object.entries(source ?? {})) {
439+
const targetValue = target[key];
440+
const canDeepMerge = isPlainObject(targetValue) && isPlainObject(sourceValue);
441+
442+
// Recurse into a fresh object so the shared defaults stay untouched; otherwise overwrite.
443+
target[key] = canDeepMerge
444+
? configMerge({}, targetValue, sourceValue)
445+
: sourceValue;
464446
}
465447
}
466-
return result;
448+
449+
return target;
467450
}
468451

469452
Module.definitions = {};

tests/unit/classes/module_spec.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,20 @@ describe("File js/module (cloneObject)", () => {
245245
expect(moduleInstance.config).toEqual({ nested: { value: 1 } });
246246
});
247247

248+
it("should overwrite object defaults with arrays in deep merge mode", () => {
249+
const moduleName = "MMM-TestDeepMergeArrayOverwrite";
250+
Module.register(moduleName, {
251+
defaults: {
252+
nested: { keep: true }
253+
}
254+
});
255+
256+
const moduleInstance = Module.create(moduleName);
257+
moduleInstance.setConfig({ nested: [1, 2] }, true);
258+
259+
expect(moduleInstance.config).toEqual({ nested: [1, 2] });
260+
});
261+
248262
it("should initialize lifecycle fields in setData", () => {
249263
const moduleName = "MMM-TestSetData";
250264
Module.register(moduleName, {

0 commit comments

Comments
 (0)