FlowFuse / node-red-dashboard

https://dashboard.flowfuse.com
Apache License 2.0
160 stars 35 forks source link

'widget-load' event sent multiple times to widget #909

Open omrid01 opened 1 month ago

omrid01 commented 1 month ago

Current Behavior

In custom dashboard 2.0 nodes, in some scenarios, the widget (client node) receives multiple identical 'widget-load' notifications

Expected Behavior

Dashboard 2.0 widgets (client nodes) should receive a single 'widget-load' notification from the server (when there is data in the Node-red data store for this node).

Steps To Reproduce

To avoid external influences, the reproduction can be done on a clean ui-example node.

  1. change the server node (ui-example.js) to set some data to the datastore upon node startup (to ensure that it will send 'widget-load' events to the clients:

    if (!base.stores.data.get(node.id))
    base.stores.data.save(base, node, "XYZ");
  2. Set a counter & printout in the widget code (UIExample.vue)

    mounted () {
    this.$socket.on('widget-load:' + this.id, (msg) => {
      this.evCount++;
      console.log(`widget-load: node=${this.id},msg=${msg},count=${this.evCount},timestamp=${Date.now()}`);
      return;
    ...   
  3. When opening or refreshing a new page, the notification arrives once, as expected: image

  4. However, if the client stays open, and we restart the Node-red server process, or deploy/restart flows in the editor, we get multiple notifications (the number of notifications increases on every deploy, and their timestamps go backwards): image

Environment

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

omrid01 commented 1 month ago

While checking this, it would be nice to modify the 'widget-load' notification behavior, so that it will always be sent (even if the datastore is empty, in which case it will have a null msg). - https://github.com/FlowFuse/node-red-dashboard/issues/793 This shall allow the widget to rely on this notification, and do initialization tasks at one point in time, based on the data (or lack of it) in the datastore.

colinl commented 1 month ago

I can confirm that I see this too. In fact it is only necessary to make a trivial change to the node and deploy it to get the accumulating widget-load messages.

colinl commented 1 month ago

By the way @omrid01, thanks for inadvertently solving the second part of my question in https://discourse.nodered.org/t/questions-about-creating-d2-ui-nodes-and-the-node-red-data-stores/88316 (why my code was not clearing the store).

joepavitt commented 3 weeks ago

Just to check, are you include the this.$socket?.off('widget-load' + this.id) in the unmounted as per the example too?

joepavitt commented 3 weeks ago

Can re-produce, even with the this.$socket.off - investigating now.

Updates:

joepavitt commented 3 weeks ago

For the core widgets, it was firing because I never actually ran socket.off for the widget-load event, adding that in does fix it for core, but why that same fix in custom nodes doesn't fire, I have not yet worked that out.

joepavitt commented 3 weeks ago

Found it - typo in the ui-example code:

this.$socket?.off('widget-load' + this.id)

is missing a colon, should be:

this.$socket?.off('widget-load:' + this.id)
joepavitt commented 3 weeks ago

I'll get the example updated, and also push in a fix for the core nodes too

bartbutenaers commented 3 weeks ago

@joepavitt, Then every developer from all third-part ui nodes in your overview should be aware of this, and implement that fix in their repo...

joepavitt commented 3 weeks ago

Good point @bartbutenaers

The third party devs I'm aware of are already mostly in this thread, but also pulling in @sumitshinde-84 for webcam widgets

colinl commented 3 weeks ago

Thanks for the heads up on this. I need to fix it in my gauge node.

bartbutenaers commented 3 weeks ago

I did fix it tonight in the svg node

omrid01 commented 3 weeks ago

same same on ui-tabulator node which will be published tomorrow

sumitshinde-84 commented 3 weeks ago

Thanks for the heads up, @joepavitt. I will take a look at the webcam widget's repo and will fix it soon.