Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not blocked "Undefined" in ui-chart legend #1575

Open
teluse7 opened this issue Jan 11, 2025 · 22 comments
Open

Not blocked "Undefined" in ui-chart legend #1575

teluse7 opened this issue Jan 11, 2025 · 22 comments
Labels
bug Something isn't working needs-triage Needs looking at to decide what to do

Comments

@teluse7
Copy link

teluse7 commented Jan 11, 2025

Current Behavior

When sending any lines in the data without the necessary keys "series" and "value" , ui-chart works correctly, but returns "undefined" in the legend.

Expected Behavior

Need ability to block "undefined" in legend

Steps To Reproduce

Make the parameter "block undefined in legend" in the ui-chart settings

Environment

  • Dashboard version:
  • Node-RED version:
  • Node.js version:
  • npm version:
  • Platform/OS:
  • Browser:

Have you provided an initial effort estimate for this issue?

I am not a FlowFuse team member

@teluse7 teluse7 added bug Something isn't working needs-triage Needs looking at to decide what to do labels Jan 11, 2025
@joepavitt
Copy link
Collaborator

You can hide the legend already in the chart settings

@teluse7
Copy link
Author

teluse7 commented Jan 12, 2025

You can hide the legend already in the chart settings

I need hide only undefined, not all legend.

@colinl
Copy link
Contributor

colinl commented Jan 12, 2025

It would be better not to send undefined to the chart.

@joepavitt
Copy link
Collaborator

Right, but the only use case where this occurs is when you have a single line of data you're passing to the chart, so there is no need for a Legend? Or, change the "Series" to a hard-coded string to give the line a label.

It is undefined, because the label for the line is quite literally undefined

@teluse7
Copy link
Author

teluse7 commented Jan 12, 2025

Right, but the only use case where this occurs is when you have a single line of data you're passing to the chart, so there is no need for a Legend? Or, change the "Series" to a hard-coded string to give the line a label.

It is undefined, because the label for the line is quite literally undefined

I have a single data flow for many diagrams and some rows for one, some rows for another one. The diagrams themselves now read their own data and do not draw other data. But "undefined" is added to such other rows in the legend. I have normal legend + "undefined". I need block this.

Example:
var output = [
{ 'series400': "Common voltage", 'values400': 401 },
{ 'series230': "Voltage A", 'values230': 235 },
{ 'series230': "Voltage B", 'values230': 230 },
{ 'series230': "Voltage C", 'values230': 232 },
{ 'series400': "Voltage A", 'values400': 402 },
{ 'series400': "Voltage B", 'values400': 400 },
{ 'series400': "Voltage C", 'values400': 401 }
];

msg.payload = output;
return msg;

Screenshot_20250112_155324_Samsung Internet
Screenshot_20250112_155256_Samsung Internet

@joepavitt
Copy link
Collaborator

Ah, okay, so you have instances where a datapoint passed to your chart doesn't actually contain any data to plot. The chart is trying, but failing to, and therefore adding an undefined series?

If so, that makes sense, thank you for clarifying, and something we should handle.

@teluse7
Copy link
Author

teluse7 commented Jan 13, 2025

Yes, you understand everything absolutely correctly now.
Exactly like that.

@teluse7
Copy link
Author

teluse7 commented Jan 14, 2025

what should I do/expect next?

@joepavitt
Copy link
Collaborator

If you're a VueJS developer, you're welcome to dive in and try to fix it. Otherwise, it'll be a matter of waiting until someone is able to pick it up.

@teluse7
Copy link
Author

teluse7 commented Jan 23, 2025

If you're a VueJS developer, you're welcome to dive in and try to fix it. Otherwise, it'll be a matter of waiting until someone is able to pick it up.

I am not a VueJS developer, I can't even imagine what this VueJS is.
But I corrected the already compiled file and now everything works correctly.
It is necessary to correct the text:
add (msg) {
const payload = msg.payload

// determine what type of msg we have
if (Array.isArray(msg) && msg.length > 0) {
// we have received an array of messages (loading from stored history)
msg.forEach((m, i) => {
const p = m.payload
const d = m
._datapoint
// server-side we compute a chart friendly format
const label = d.category
this.addPoints(p, d, label)
})
} else if (Array.isArray(payload) && msg.payload.length > 0) {

// we have received a message with an array of data points
// and should append each of them
payload.forEach((p, i) => {
const d = msg
._datapoint ? msg._datapoint[i] : null
// server-side we compute a chart friendly format where required
const label = d.category
this.addPoints(p, d, label)
})
} else if (payload !== null && payload !== undefined) {

// we have a single payload value and should append it to the chart
const d = msg._datapoint
// server-side we compute a chart friendly format
const label = d.category
this.addPoints(msg.payload, d, label)

to:
add (msg) {
const payload = msg.payload

// determine what type of msg we have
if (Array.isArray(msg) && msg.length > 0) {
// we have received an array of messages (loading from stored history)
msg.forEach((m, i) => {
const p = m.payload
const d = m
._datapoint
// server-side we compute a chart friendly format
const label = d.category
if (label !== null && label !== undefined) {
this.addPoints(p, d, label)
}
})
} else if (Array.isArray(payload) && msg.payload.length > 0) {

// we have received a message with an array of data points
// and should append each of them
payload.forEach((p, i) => {
const d = msg
._datapoint ? msg._datapoint[i] : null
// server-side we compute a chart friendly format where required
const label = d.category
if (label !== null && label !== undefined) {
this.addPoints(p, d, label)
}
})
} else if (payload !== null && payload !== undefined) {

// we have a single payload value and should append it to the chart
const d = msg._datapoint
// server-side we compute a chart friendly format
const label = d.category
if (label !== null && label !== undefined) {
this.addPoints(msg.payload, d, label)
}

in the file:
https://github.com/FlowFuse/node-red-dashboard/blob/main/ui/src/widgets/ui-chart/UIChart.vue

I.e. You need to add if (label !== null && label !== undefined)

@bartbutenaers
Copy link
Contributor

I am not a VueJS developer, I can't even imagine what this VueJS is.

But at least you tried to find a solution, and it seems you even managed to fix it. Kudos!!
Such kind of help is very welcome here, because you pinpointed the location in the code where things go wrong.

By looking at your fix, it "looks" to me that you found an issue that is not only related to the legend.
In your example you have datapoints that don't contain the key property, which has been specified in the config screen.
While your fixed version is very simple and neat, these datapoints (without the key property) are still being pushed to the frontend, where you skip them.

But in fact it is quite a big overhead that the chart node stores all these faulty datapoints in its memory, and pushes all these datapoints to the frontend where they will be skipped. Moreover after a refresh of the browser page, I assume all these datapoints will be replayed and ignored again. At the end you managed to keep them away from your dashboard ui, but under the cover they will still be used all over the place. That probably won''t be a problem for your use case, but it will be quite a resource bottleneck when a lot of such faulty datapoints are being injected...

I have not tested this, but based on your fix code I "think" you can drop all these faulty datapoints as soon as they are being injected into the ui-chart node in the server-side code. Something like this I think, where I have added my comments in capitals, of code I "think" needs to be changed.:

  let series = RED.util.evaluateNodeProperty(config.category, config.categoryType, node, msg)
  // if receiving a object payload, the series could be a within the payload
  if (config.categoryType === 'property') {
      series = getProperty(p, config.category)
  }

  // single point or array of data?
  if (Array.isArray(p)) {
      // array of data
      msg._datapoint = p.map((point) => {
          // series available on a msg by msg basis - ensure we check for each msg
          if (config.categoryType === 'property') {
              series = getProperty(point, config.category)
          }

          // IF THE KEY IS NOT AVAILABLE THEN SKIP THIS DATAPOINT
          if (!series) {
              return null
          }

          return addToChart(point, series)
      }).filter((point) => point !== null) // REMOVE THE NULL VALUES FROM THE LIST, WHICH YOU HAVE  RETURNED ABOVE
  } else {
      // single point
      if (config.categoryType === 'json') {
          // we can produce multiple datapoints from a single object/value here
          const points = []

          // MAKE SURE THE SERIES ARE NOT UNDEFINED
          if (series) {
              series.forEach((s) => {
                  if (s in p) {
                      const datapoint = addToChart(p, s)
                      points.push(datapoint)
                  }
              })
          }
          msg._datapoint = points
      } else {
          // MAKE SURE THE SERIES ARE NOT UNDEFINED
          if (series) {
              msg._datapoint = addToChart(p, series)
          }
      }
  }

However by doing it this way, the msg._datapoint might now become undefined or [] by skipping the datapoints without the key property. That means we have to be aware of that on the frontend-side code also, to make sure there is no crash:

        add (msg) {
            const payload = msg.payload
            // determine what type of msg we have
            if (Array.isArray(msg) && msg.length > 0) {
                // we have received an array of messages (loading from stored history)
                msg.forEach((m, i) => {
                    // SKIP THE DATAPOINT IF NOT AVAILABLE
                    if (m._datapoint) {
                       const p = m.payload
                       const d = m._datapoint // server-side we compute a chart friendly format
                       const label = d.category
                       this.addPoints(p, d, label)
                    }
                })
            } else if (Array.isArray(payload) && msg.payload.length > 0) {
                // we have received a message with an array of data points
                // and should append each of them

                // SKIP THE DATAPOINTS IF NOT AVAILABLE
                if (msg._datapoint && Array.isArray(msg._datapoint) && payload.length === msg._datapoint.length) {
                    payload.forEach((p, i) => {
                        // CODE SIMPLIFIED BECAUSE WE KNOW THAT THE DATAPOINT EXISTS HERE
                        const d = msg._datapoint[i] // server-side we compute a chart friendly format where required
                        const label = d.category
                        this.addPoints(p, d, label)
                    })
                }
            } else if (payload !== null && payload !== undefined) {
                // SKIP THE DATAPOINT IF NOT AVAILABLE
                if (msg._datapoint) {
                    // we have a single payload value and should append it to the chart
                    const d = msg._datapoint // server-side we compute a chart friendly format
                    const label = d.category
                    this.addPoints(msg.payload, d, label)
                }
            } else {
                // no payload
                console.log('have no payload')
            }
            if (this.chartType === 'line' || this.chartType === 'scatter') {
                this.limitDataSize()
            }
            this.update()
        },

Don't hesitate to aks me if something is not clear. I have not tested this code, due to lack of free time!!

Bart

@teluse7
Copy link
Author

teluse7 commented Jan 24, 2025

Don't hesitate to aks me if something is not clear. I have not tested this code, due to lack of free time!!

if you delete on the server side (I did it at beginning), then the output of the diagram will not get the original flow, but a truncated one and the next diagram or next some other node will not receive the necessary data. you definitely don't need to do this!!

I sent the correct code that does not spoil the flow

@bartbutenaers
Copy link
Contributor

Ah yes, good point.

As mentioned above I have not yested my code. But at first sight I wouldn't expect my code to change the original input message. So if you enable passthrough, I would expect the next nodes in your flow to get all the original data.

Please correct me if I am wrong. I don't know the internals completely for this chart node, so perhaps the modified data somehow finds its way to the output...

@teluse7
Copy link
Author

teluse7 commented Jan 24, 2025

Ah yes, good point.

As mentioned above I have not yested my code. But at first sight I wouldn't expect my code to change the original input message. So if you enable passthrough, I would expect the next nodes in your flow to get all the original data.

Please correct me if I am wrong. I don't know the internals completely for this chart node, so perhaps the modified data somehow finds its way to the output...

How can I make sure that at least some kind of fix is taken in release ?

@bartbutenaers
Copy link
Contributor

You will need to get it into a pull request, so the Flowfuse people can review and merge it into the master branch of this repository. Take into account that they are currently very busy (on other non-dashboard related stuff) so not sure when they will get to it.

Meanwhile - since you have already experimented to filter the datapoints on the server side - it would be very useful if you could also post here how you did that, and why it didn't work. As described in my last post, I don't really understand at the moment why that wouldn't work. Your feedback would really be helpful for any contributor that wants to implement server-side filtering in the future. Thanks!!

@teluse7
Copy link
Author

teluse7 commented Jan 24, 2025

You will need to get it into a pull request, so the Flowfuse people can review and merge it into the master branch of this repository. Take into account that they are currently very busy (on other non-dashboard related stuff) so not sure when they will get to it.

Meanwhile - since you have already experimented to filter the datapoints on the server side - it would be very useful if you could also post here how you did that, and why it didn't work. As described in my last post, I don't really understand at the moment why that wouldn't work. Your feedback would really be helpful for any contributor that wants to implement server-side filtering in the future. Thanks!!

Unfortunately I can't push, I was asked to help, find a place and fix it, I found it, but maybe your version is better.
When I made empty datapoint strings on the server side, I got an error that somewhere they were accessing a property of a non-existent object. This error was not on the client, but on the server and was output to the node-red console. Component did not work and did not have output flow.

When I made datapoints smaller, only in terms of data quantity, everything in the component moved apart.
Then I had to delete the lines from msg and not write to datapoints at the same time.
It's work, but the output flow was corrupted

In theory, your version with empty datapoints should cause an error somewhere on the server side.

@bartbutenaers
Copy link
Contributor

Thanks for your explanation! That will be helpful if anybody wants to look at this in more depth in the near future.

Unfortunately I can't push

Do you mean you need help to create a pull request for your workaround? If yes, I can explain the basic steps.

@teluse7
Copy link
Author

teluse7 commented Jan 26, 2025

Do you mean you need help to create a pull request for your workaround? If yes, I can explain the basic steps.

If you can make this request, that would be great.
It would be impossible for me to do it from my phone.

I don't claim any copyright.

@bartbutenaers
Copy link
Contributor

@teluse7
Here you go.

I don't claim any copyright.

First I thought: lol, that is a good one.
But indeed I had signed an agreement at the time being, that I don't claim any copyright.
So good from you to mention it!!

Now you have to wait until Joe has time to review and merge it, but he is very busy at the moment with other stuff unfortunately...

@teluse7
Copy link
Author

teluse7 commented Jan 27, 2025

@teluse7 Here you go.

I don't claim any copyright.

First I thought: lol, that is a good one. But indeed I had signed an agreement at the time being, that I don't claim any copyright. So good from you to mention it!!

It wasn't about copyright in the legal sense. I just said that I don't know how to push and most likely I won't be able to technically. But if you can do it, that would be great. I don't claim that it will be written that I found a bug. If you push and it will be considered that you found a bug, that's also great. That's what I'm talking about. I helped as much as I could.

@teluse7
Copy link
Author

teluse7 commented Jan 31, 2025

Now you have to wait until Joe has time to review and merge it, but he is very busy at the moment with other stuff unfortunately...

Hello.
I saw that this change was added somewhere, but there was an error in the tests. It seems that the tests concern something else. Need to find another error?

@bartbutenaers
Copy link
Contributor

@teluse7
Very often the unit tests fail due to something unrelated, i.e. in tests of another ui widget.
This time however one of the failing tests is inside the ui-chart node itself:

Image

So I first thought that your fix perhaps somehow caused that test to fail.
That failing unit test seems to use this test flow.
When I import that test flow in my Node-RED manually and I test that flow I indeed get a chart containing only a single point:

Image

But the failing unit test is not caused by your fix, because I have run it on a Node-RED instance that doesn't contain your fix.
So no idea why the test fails. I don't have enough free time to figure that all out on my own...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-triage Needs looking at to decide what to do
Projects
Status: Backlog
Development

No branches or pull requests

4 participants