Notification on subsequent DOM updates (not just the first)

[ryan-d-williams]

Based on the documentation and by taking a look at the source code, right now the MODULE_DOM_CREATED notification is fired when the module is first loaded. Subsequent calls to updateDom do not fire any event.

I think it would be very beneficial to add a notification for each time the DOM is updated. I am currently accomplishing this in a vary hacky way, but it would be nice to add a notification to know exactly when the DOM update is complete so the module can interact with the new DOM if necessary.

My proposal is to add MODULE_DOM_UPDATED as a notification that fires when calls to updateDom are made.

The same code that is used to send the MODULE_DOM_CREATED notification is also used during updateDom, so the code changes will be minimal.

I'm happy to submit a PR for this change, but wanted to discuss it here first.

[sdetweil]

who is updating your dom except you???

this was just the system letting you know that SOMETIME after your first getDom() return the content is IN the DOM so if you

use getDocumentBy????? they would be there

BEFORE the first notification , they are NOT in the dom..

this notification helps you by not having to set a timer for unknown length, or repeat checking

[sdetweil]

calls to updateDom are syncronous in that they call getDom() or getTemplateData() before returning.. (after the specified delay. if any)

the problem on the 1st is YOU didn't call updateDom().

[ryan-d-williams]

@sdetweil thank you for your response and for looking into this with me!

If the expectation is that updateDom is synchronous, then I believe there is a bug. See this example:

this.updateDom(0);
console.log("Just after updateDom");
console.log(document.getElementsByClassName("table-wrapper"))

setTimeout(() => {
    console.log("500ms Delay");
    console.log(document.getElementsByClassName("table-wrapper"))
}, 500)

If updateDom was synchronous, it would be expected that both of those log statements would return the same thing (since the content should be rendered after updateDom). However, they do not return the same thing:

This means that there is some unspecified delay after updateDom before the content is on the screen. Note that this is the same result even if updateDom is awaited.

I agree that if updateDom is synchronous, then there is not a need for MODULE_DOM_UPDATED, but it does not appear to be designed as synchronous our behaving synchronously. I think once it is determined how that function is expected to behave will determine the next steps. If it is expected to be synchronous then there is a bug. If it is expected to be async then MODULE_DOM_UPDATED will be useful.

Thanks again for your reply!

[sdetweil]

yeh, looking at the code that was added for the transitions its become async, and not await able,
so now your problem appears

[ryan-d-williams]

That's what I'm seeing too.

So it seems as though adding in the MODULE_DOM_UPDATED is a simple change to ensure the module can still know when the DOM is updated without having to update that entire function stack to make it synchronous. I did a quick test and was able to add this notification using only a few extra lines (in the same manner that MODULE_DOM_CREATED is already emitted).

Should I go ahead with a PR so we can all see the potential changes?

[sdetweil]

i would

[sdetweil]

i wonder how many modules use this notification incorrectly

[ryan-d-williams]

Which notification are you referring to? I think MODULE_DOM_CREATED is emitted correctly (but only emitted once). Is there a chance modules are using it incorrectly currently?

[sdetweil]

yes, we have unknown skill developers.

[ryan-d-williams]

Good point. Once I get the PR submitted I can work on a PR for the docs to make this new notification known and the old one more explicit.

[sdetweil]

@KristjanESPERANTO what would it take to use the module list to scan for this notification literal

[KristjanESPERANTO]

@sdetweil I can search for strings in all modules relatively easily. These modules are containing the string MODULE_DOM_CREATED:

1. EXT-Pages: if (notification === "MODULE_DOM_CREATED") {
2. internet-monitor: if (notification === "MODULE_DOM_CREATED") {
3. MMM-anotherNewsFeed: if (notification === "MODULE_DOM_CREATED" && this.config.hideLoading) {
4. MMM-BlaguesAPI: if (notification === 'MODULE_DOM_CREATED') {
5. MMM-CalendarExt3Agenda: if (notification === 'MODULE_DOM_CREATED') {
6. MMM-Carousel: if (notification === "MODULE_DOM_CREATED") {
7. mmm-dropbox: if (notification === 'MODULE_DOM_CREATED') {
8. MMM-Markdown: if (notification === 'MODULE_DOM_CREATED') {
9. MMM-MercedesMe: else if (notification === 'MODULE_DOM_CREATED') {
10. MMM-MeteoFrance: case "MODULE_DOM_CREATED":
11. MMM-MPRIS2-WebSocket: if (notification == "MODULE_DOM_CREATED") {
12. MMM-MusicButler: if (notification === "MODULE_DOM_CREATED" && this.config.hideLoading) {
13. MMM-neoomAPI: if (notification === 'MODULE_DOM_CREATED') {
14. MMM-OnThisDay: if (notification === 'MODULE_DOM_CREATED') { and module.notificationReceived('MODULE_DOM_CREATED');
15. MMM-Page-Selector: }else if(notification === "MODULE_DOM_CREATED"){
16. MMM-PlexNowPlaying: case "MODULE_DOM_CREATED":
17. MMM-RandomPhoto: if (notification === "MODULE_DOM_CREATED") {
18. MMM-RemoteCompliments: if (notification === "MODULE_DOM_CREATED") {
19. MMM-Speedway: if (notification === 'MODULE_DOM_CREATED') {
20. MMM-Tronity: else if (notification === 'MODULE_DOM_CREATED') {
21. MMM-Wallpaper: if (notification === "MODULE_DOM_CREATED" && self.config.userPresenceAction === "show") {
22. MMM-WiFiPassword: case "MODULE_DOM_CREATED":
23. MMM-YouTubeWebView: if (notification == 'MODULE_DOM_CREATED') {

I do not yet understand how to recognize whether this notification could be used incorrectly. If this is easy, maybe I could add a check for it in the list.

[sdetweil]

after use is harder, if the module used document.getElementsBy... functions then its possible they could be impacted

if not then no impact

[sdetweil]

i should have also added! NICE that you can search!!!

[ryan-d-williams]

I'm not sure if it is possible to detect incorrect usage using static analysis only.

For example, a module could use getElementsBy... or getElementBy... and still use the MODULE_DOM_CREATED correctly (for example if they never call updateDom or if they only care about the first time the DOM was rendered).

You would have to able to determine that the module 1. Calls updateDom and 2. Intends to see the re-render via MODULE_DOM_CREATED. I don't think you can detect both of those conditions statically.

I also don't think we need to detect this condition. If a user was using it incorrectly, their module simply wouldn't work (they would be waiting for a notification that never arrives).

[KristjanESPERANTO]

Okay, thank you both for the explanation 🙂 I drop the idea of trying to check this.
At least you have now an overview of how many modules use this notification. Feel free to let me know if you have similar questions in the future.

[sdetweil]

well, can you do another or nested search

search those modules for

document.getElementsby

only care about the ones that have it

[ryan-d-williams]

@sdetweil even with a nested search, there are still many ways those two things can be nested and still be used correctly.

If I wait for the MODULE_DOM_CREATED notification, it just means the DOM has been rendered the first time. I can safely use document.getElementsBy... or any other variation.

The danger would be if I tried to use a combination of updateDom and MODULE_DOM_CREATED assuming that the latter was fired on a DOM re-render. But since there are valid reasons to use this notification, I don't think it's possible to determine it is being used incorrectly by string search (you need to understand the intent of it).

For example, a module can use MODULE_DOM_CREATED to do something on the first render, and then later on call updateDom without caring what happens on the re-render and both of those are independently safe operations. Any nested string search would flag this safe operation.

[sdetweil]

doah.. true

[sdetweil]

as you are proposing a new notification and not overloading the old, it doesn't matter

[ryan-d-williams]

@sdetweil @khassel @rejas

Sorry to rehash this... while I was submitting a PR for the doc update, I realized that my choice of notification name was not ideal.

Given the 3 current notifications (ALL_MODULES_STARTED, DOM_OBJECTS_CREATED, MODULE_DOM_CREATED), it would make much more sense for this new notification to be MODULE_DOM_UPDATED instead of the current DOM_OBJECTS_UPDATED since the update is only for the requesting module and is only sent to one module.

I went ahead and added a new commit/PR. Let me know if you have any thoughts or feedback.

[sdetweil]

good catch. i agree