Modules with no header are always redrawn in updateDom, even when module content is unchanged

[benjaminflessner]

I found a bug in MagicMirror

Platform: Mac, Chrome

Node Version: 18

MagicMirror² Version: Latest khassel docker image

Description:
Modules with no header seems to be always refreshing, even if content did not change.

Steps to Reproduce:
Set any module to a low-update frequency, and lose the header. (Easier to see if the module calls updateDom with a visible fade)

Notice that the module refreshes every time, even if its contents do not change.

Expected Results:
Expect to see the module NOT refresh unless data changes.

Actual Results:
Module refreshes constantly!

Additional Notes:
First traced down to

MagicMirror/js/main.js

Line 224 in 5ea8a34

headerNeedsUpdate = newHeader !== headerWrapper[0].innerHTML;

where newHeader === undefined and headerWrapper[0].innerHTML === "undefined" (the string). Looking at my page source - this is indeed correct!

<header class="module-header" style="display: none;">undefined</header>

The innerHTML is literally the string "undefined." But why?

Traced that to

MagicMirror/js/main.js

Line 37 in 5ea8a34

moduleHeader.innerHTML = module.getHeader();

which converts the undefined return of getHeader() to the string "undefined" in innerText.

.... then I got distracted by a rabbit trail trying to figure out how the heck the following line worked at all. Shouldn't it set the style.display to none if getHeader was ==="" not !==""?

MagicMirror/js/main.js

Line 41 in 5ea8a34

if (typeof module.getHeader() === "undefined" || module.getHeader() !== "") {

I think the only reason things work is because updateModuleContent re-sets the display style to block here:

MagicMirror/js/main.js

Line 251 in 5ea8a34

headerWrapper[0].innerHTML = newHeader;

I'd be happy to submit a PR for this, but I'm a bit concerned I'm mis-understanding what's going on in line 41. The easy fix is to change moduleNeedsUpdate to set headerNeedsUpdate to false when newHeader === undefined && headerWrapper[0].innerHTML === "undefined". But it would be cleaner to not be sticking the string "undefined" into the DOM at all. Should probably just be setting the innerHTML to an empty string in this instance, and then handling that in moduleNeedsUpdate.

[sdetweil]

undefined in this context means not set.

the module calling updateDom() says the content changed. it's the module's job to tell MagicMirror. we don't compare old vs new.

[sdetweil]

in modules where I don't want the change flash, I return the same Dom object tree I built last(first) time. so the replace is identical.

the browser knows if the dom elements are different or not.

more and more the browsers are implementing a shadow Dom. memory access is significantly faster than screen drawing,
so they draw to a shadow compare and draw diffs to the screen

[benjaminflessner]

the module calling updateDom() says the content changed. it's the module's job to tell MagicMirror. we don't compare old vs new.

updateDom calls updateDomWithContent which literally calls moduleNeedsUpdate whose sole purpose is to compare the header and body of the new module content with the current content to determine if an update is needed. It just doesn't work when the header is null because undefined !== "undefined".

[sdetweil]

cool.. I am wrong.. so, edit the code and try it out.. see what happens.. all you have to do is remove 2 quotes..

you can use the developers window (ctrl-shift-i) , sources tab , to put a stop on this line of code and checkout the behavior now
i don't see any difference.. that line of code is called once at startup of module..

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.