Magicmirror: Module config not deep merging.

[MichMich]

See issue #2113.

[rejas]

A good place for the code would be in the Utils class, I think.

[MichMich]

(I removed some off-topic comments to keep this issue clean. Feel free to open a separate issue to discuss further.)

[MichMich]

A good place for the code would be in the Utils class, I think.

Agree!

[sdetweil]

how do I do this with utils? only place utils is used is on the server side.., using require(utils.js)

    setConfig: function (config) {
        //this.config = Object.assign({}, this.defaults, config);
        this.config = Utils.configMerge({}, this.defaults, config);
    },

gets Utils not defined in browser

[MichMich]

We can just import utils in the browser just like any other js file.

[rejas]

We could if there wasn't the colours dependency in there. Either rework that or use a browser-require solution...

[MichMich]

Or take a simple route: BrowserUtils.js :)

[sdetweil]

vs just add it to the module.js? it's only appropriate in this one context

[rejas]

@sdetweil true

[MichMich]

Agee. If is is used in other spaces as wall later, we can always refactor it. 👍🏻

[sdetweil]

so, can we reopen pr 2114? which does that?

[MichMich]

Reopened it. Will take a look later.

[sdetweil]

thanks, I don't see PR open

[bugsounet]

Or take a simple route: BrowserUtils.js :)

it will be difficult ... you can't require any file

better way, include it in modules.js like cmpVersions()

[sdetweil]

@bugsounet we got it back to your submission. all good

[bugsounet]

I know ... I try many solutions but not works ... the require function of electron is disabled by default
and it's not a solution to enable it

[MichMich]

Can't we just copy lodash's implementation into the module.js file? That way we don't have to require or import anything.

[bugsounet]

sure electron security ... ;(

after... it's my point of view ... i don't want to enforce security

[sdetweil]

@MichMich lodash etc. either way u pick up code ownership and maint responsibility.

we have something, why go get something else?

[bugsounet]

Can't we just copy lodash's implementation into the module.js file? That way we don't have to require or import anything.

or maybe in async function before loading the config ... but not sure

[MichMich]

Sam, I don't think what you are saying. Is adding the lodash code a valid option or do you have a better suggestion?

[sdetweil]

@MichMich I haven't looked at copying the lodash code into the module.js file.

in either case you will own whatever code gets included.

we have a solution, in pr form, why not use it
I suggested lodash if we could require() it, but can't. I don't plan to evaluate copying the lodash code, as the result will be the same: a new routine in module.js, which the mm project will own

[MichMich]

Ah, ok. Merging the PR seems fine by me, but if I understand @bugsounet's comment correctly that PR doesn't work with Electron?

[bugsounet]

no, no

I says ... you can't include a require() function in Module Class. it's the security of electron (you can't use external code with require() )

[sdetweil]

I replicated his PR. it works fine.

[sdetweil]

@bugsounet not just electron. all browsers removed local file access a long time ago due to hacking

[bugsounet]

sure agree :+1:

in webPreferences
nodeIntegration Boolean (optional) - Whether node integration is enabled. By default the value is false.

that why we can't use require function()

That why I patch module.js directly

[MichMich]

Ok, so would reopening your PR and and merging it be a valid option, @bugsounet?

[bugsounet]

I don't know... It's not my repo

[Censured by me.. but you have the mail]

[MichMich]

GitHub won't let me reopen the PR. So we will need to make a new one. Will look into it as soon as I have time.

Thanks for your help and suggestion @bugsounet.

[bugsounet]

1) If you don't want to respond in public ... find my mail ...
2) because i'm nice ... i will recreate it... but: don't count on me for other PR... (you know why... see your mail)

[MichMich]

So, @rejas brought a valid point to my attention. In some cases you want a deep merge of a config. But in a lot of cases, you want to just simply replace the existing object or array. So simply replacing the current implementation with a deep merge will lead to unexpected results for a lot of configurations and modules.

For example: the example configs for newsfeed and compliments would give unexpected results with the deep merge strategy.

I like @bugsounet's implementation of the deep merge. But applying it to all config elements is probably not what most users and module developers want. So before we merge the PR, we need to think of alternative strategies. For example: let developers decide which config kees need a deep merge. This is powerful, bust complicates a lot of things.

@bugsounet, @sdetweil I'm curious to what your thoughts are on this subject.

[bugsounet]

I have another idea;

If developer want to use mergeConfig script
I can define a data value in the begining of the module like position, module name

something like:

module: "MMM-Sonmething",
position: "top_center",
configMerge: true,
config: {...},

of course by default, it's false
and apply it or not in setData: function (data) {} (from module .js)

[MichMich]

That sounds like a great alternative. Especially since it can be extended later to include a array or object to define which objects should be merged. A Boolean version is enough for now and covers most of the issues, I think!

Agree, @sdetweil @rejas ?

[sdetweil]

I think that sounds like a great choice.. each developer doesn't have to come up with their own solution to cover user problems, there is a consistent implementation, and easy use... I'm for it..

[MichMich]

One suggestion: maybe we should call the variable configDeepMerge: true, since it's merged by default and deep merged by this implementation?

[sdetweil]

sure, that will make it a tiny bit harder to extend is a non-volatile way. but who know when that might be

[bugsounet]

I will try code code something to see react of MM and make some test in local...

[maximiliano2018]

Hi guys, glad to see that you are reaching an agreement, I am not a developer but what a great job everyone has done here, @MichMich, @bugsounet, @eouia .

“If we are together there is nothing impossible. If we are divided, everything will fail ", Winston Churchill.

“Getting together is the beginning. Keeping together is progress. Working together is success ”, Henry Ford

Know that you are fabulous, each and every one of you.
We have something very good here!
Let's keep going!

Keep in mind that;
"Talent wins games, but teamwork and intelligence win championships", Michael Jordan.

And sorry for diverting the thread of the conversation.
Sincerely.
Max.

[rejas]

Agree, @sdetweil @rejas ?

Agree :-) Good idea!

[MichMich]

Merged your PR, @bugsounet! Thanks, hopefully this covers te edge cases.

Can I close this issue?

[bugsounet]

No problem, I hope this tool will help others developers ;)

I will don't come back to the subject ...

Good continuation from your side, I am leaving now from mine ;)