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

Dynamic Options - Implement new helper functions & update widget-load behaviour #1123

Merged
merged 11 commits into from
Aug 7, 2024

Conversation

joepavitt
Copy link
Collaborator

@joepavitt joepavitt commented Jul 24, 2024

Description

Okay, this is a fairly big shift, so will do my best to articulate the changes, and the why.

widget-load changes

  • We now include a state value in the widget-load event. This is because any changes, and dynamic properties, set on a node when not in view on the Dashboard would be updated server-side, but not client-side. So, when a user navigates (via the side navigation) to the relevant page, the state loaded is still the original configuration provided by Node-RED's latest deploy.
  • In the client-side handler for this event, we now automatically put everything received in state straight into our VueX store under the widget.state field. This means that we don't need to define this on every widget, which we would need to. Note though, that we still need to have the server-side definitions over what can go into state, and the client-side widgets need to reference this still via computed variables (explained next)
  • This event will now fire for every widget, as long as it has msg or state saved in the server-side stores. Previously, it would only fire if something was present in datastore (i.e. last received msg)

Move to a centralised store model

  • I've removed the use of the dynamic design pattern that we had implemented in order to track the dynamic overrides.
  • Instead, we're referencing the widget state stored in vuex directly using computed variables. This ensures we have a single source of truth, and not many copies of the truth distributed out to each widget/node.

New global helper functions

  • In order to make the interface to the vuex store easier, I've created two new global helper functions for dealing with dynamic (and static) properties:
    • updateDynamicProperty(property, value): Updates a property with the provided value, and checks against undefined types
    • setDynamicProperties(config): Will set the provided properties in the vuex store for a given widget this is run on. This will automatically update the widget's state, and any references using this property.
    • getProperty(property): Automatically gets the correct value for the requested property. Will first look in the dynamic properties, and if not found, will default to the static configuration (i.e. what's defined in Node-RED) that arrived via the ui-config event.

widgetState store mutation changes

  • The arguments for the widgetState mutation on the ui vuex store has changed to permit separation of the proeprties being set and the id of the widget.

Progress

We need to sweep over all of the existing widgets that were already using the design pattern to ensure consistency:

  • UI Form
  • UI Button
  • UI Button Group
  • UI Dropdown
  • UI Slider

We can then continue #833 with the new pattern introduced here

Related Issue(s)

Closes #793
Closes #1022
Closes #1117
Closes #1122

Need to double check once closed on whether it helps #871

@joepavitt joepavitt changed the title Dynamic Options - Add new helpers, improve state loading via widget-load Dynamic Options - Implement new helper functions & update widget-load behaviour Jul 24, 2024
@joepavitt
Copy link
Collaborator Author

joepavitt commented Jul 24, 2024

Only ui-dropdown to go, which is a little trickier as has some backward compatibility stuff left over with msg.options that we still need to support - will attack that tomorrow

Update: done

@colinl
Copy link
Contributor

colinl commented Jul 30, 2024

How are validations being done on ui_update operations, to make sure invalid data are not allowed?

@joepavitt
Copy link
Collaborator Author

How are validations being done on ui_update operations, to make sure invalid data are not allowed?

Needs to be done on a node-by-node basis. Right now, not much validation.

Also PR opened for UI Example updates: FlowFuse/node-red-dashboard-2-ui-example#17 will get it reviewed once these core PRs are merged

@arturv2000
Copy link
Contributor

One question regarding the use of the dynamic properties, is it sensible that the user may want to return a specific property to its default value, (for example button color, that was defined in the config window as yellow, or the label text) by sending a null or an undefined?

@joepavitt
Copy link
Collaborator Author

joepavitt commented Jul 30, 2024

One question regarding the use of the dynamic properties, is it sensible that the user may want to return a specific property to its default value, (for example button color, that was defined in the config window as yellow, or the label text) by sending a null or an undefined?

If the value is set to null, then the property is reset. Our getProperty logic checks:

return this.state && property in this.state && state !== null ? state : config

Note, if state === null, then the original config will be returned.

Each node can also have bespoke handelrs there too, e.g. ui-chart which allows reset via []

@arturv2000
Copy link
Contributor

I was testing that scenario, and it doesn't work.

For example:

  • I have a button that I defined in the config as the color is yellow and perform a full-deploy.
  • I send a message setting the color to red using ui_update.buttonColor
  • In this case, if I send ui_update.buttonColor = null the button color will return to yellow
  • If I refresh the page, the button color return to the theme default, if send again the ui_update.buttonColor = null it has no effect. If before I refresh the page i set the color to green after the refresh sending a null will return the color to green.

Not a big issue from my part, but may be something to consider if it is necessary to have a way to revert to initial configuration.

@joepavitt
Copy link
Collaborator Author

I'll handle this in a follow up @arturv2000 as I don't want to block this PR for that feature

@joepavitt joepavitt requested a review from Steve-Mcl July 30, 2024 12:36
@joepavitt joepavitt marked this pull request as ready for review July 30, 2024 12:40
@joepavitt
Copy link
Collaborator Author

Will follow up with outstanding nodes in #833 once this is merged

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Jul 31, 2024

ui-dropdown

SQLiteStudio_mYtMWIPAd0

  1. Setting label dynamically works but reverting by sending null doesnt appear to work as exepcted
  2. Dynamically setting multiple true/false causes odd entries in the current selection

chrome_Yirme9hgqh

  1. After applying dynamic label a refresh forgets selection

demo flow

[{"id":"977cfeda54f8cdd2","type":"inject","z":"953fa035e1275ec2","name":"ui_update.multiple true","props":[{"p":"ui_update.multiple","v":"true","vt":"bool"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1530,"y":340,"wires":[["0db6c5edcd4f2746"]]},{"id":"c06b96f7b431d7c0","type":"inject","z":"953fa035e1275ec2","name":"ui_update.multiple false","props":[{"p":"ui_update.multiple","v":"false","vt":"bool"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1540,"y":380,"wires":[["0db6c5edcd4f2746"]]},{"id":"0473f48afab9dff1","type":"inject","z":"953fa035e1275ec2","name":"ui_update.label null","props":[{"p":"ui_update.label","v":"null","vt":"json"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1550,"y":200,"wires":[["f6b1a8a91ef165a4"]]},{"id":"e3f54d29f355cfe6","type":"inject","z":"953fa035e1275ec2","name":"ui_update.label fred","props":[{"p":"ui_update.label","v":"fred","vt":"str"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1560,"y":240,"wires":[["f6b1a8a91ef165a4"]]},{"id":"a71cd18ca9cc520b","type":"debug","z":"953fa035e1275ec2","name":"debug 186","active":true,"tosidebar":true,"console":false,"tostatus":true,"complete":"payload","targetType":"msg","statusVal":"payload","statusType":"auto","x":1830,"y":340,"wires":[]},{"id":"a72bf45e6de20235","type":"ui-dropdown","z":"953fa035e1275ec2","group":"5c9407416fba1d1b","name":"","label":"Select Options...","tooltip":"","order":1,"width":0,"height":0,"passthru":false,"multiple":false,"chips":false,"clearable":false,"options":[{"label":"one","value":"1","type":"str"},{"label":"two","value":"2","type":"str"}],"payload":"","topic":"topic","topicType":"msg","className":"","typeIsComboBox":true,"x":1840,"y":260,"wires":[["a71cd18ca9cc520b"]]},{"id":"f6b1a8a91ef165a4","type":"junction","z":"953fa035e1275ec2","x":1680,"y":220,"wires":[["4130a873cc35b1a3"]]},{"id":"0db6c5edcd4f2746","type":"junction","z":"953fa035e1275ec2","x":1680,"y":340,"wires":[["4130a873cc35b1a3"]]},{"id":"4130a873cc35b1a3","type":"junction","z":"953fa035e1275ec2","x":1720,"y":260,"wires":[["a72bf45e6de20235"]]},{"id":"5c9407416fba1d1b","type":"ui-group","name":"PR-1123","page":"868aaffa6da2e1ce","width":"6","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"868aaffa6da2e1ce","type":"ui-page","name":"PR-1123","ui":"22ea43815413e748","path":"/page13","icon":"home","layout":"grid","theme":"c2ff5ba1f92a0f0e","order":1,"className":"","visible":"true","disabled":"false"},{"id":"22ea43815413e748","type":"ui-base","name":"base","path":"/dashboard","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"navigationStyle":"default","titleBarStyle":"default"},{"id":"c2ff5ba1f92a0f0e","type":"ui-theme","name":"Default","colors":{"surface":"#ffffff","primary":"#0094ce","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"},"sizes":{"pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]

@joepavitt
Copy link
Collaborator Author

UI Dropdown:

  1. We don't support that currently, will be a follow up exercise.

  2. If you're referring to the vertical alignment problem, that's fixed in a PR that Gayan opened yesterday, it was an underlying Vuetify bug that's fixed in newest version.

  3. I'd expect selection to be forgotten anyway on refresh? Unless the last message received contains the payload it's a shortfall of the "replay last message" approach we've copied from Dashboard 1.0

@Steve-Mcl
Copy link
Contributor

2. If you're referring to the vertical alignment problem, that's fixed in a PR that Gayan opened yesterday, it was an underlying Vuetify bug that's fixed in newest version

No, i am referring to what happens in the GIF ("two" is selected, I inject msg.ui_update.multiple = true, "two two" is now shown!)
image
I posted the demo flow too.

  1. We don't support that currently, will be a follow up exercise.

Don't support dynamic label for ui-dropdown?
As in not documented right?
(because is does work and there is code for it in this PR - so I tested it)

3. I'd expect selection to be forgotten anyway on refresh? Unless the last message received contains the payload it's a shortfall of the "replay last message" approach we've copied from Dashboard 1.0

Is this something we can fix/improve?

@colinl
Copy link
Contributor

colinl commented Jul 31, 2024

Is this something we can fix/improve

Is the problem here that a message containing only ui_update settings is stored as the last message? I think that such messages should not be stored, as the ui_update data are stored in the state store.

@joepavitt
Copy link
Collaborator Author

Don't support dynamic label for ui-dropdown?
As in not documented right?

The use of null to clear it, which is what I thought is what you're saying was not working?

@joepavitt
Copy link
Collaborator Author

Now reliving what you mean re: multiple. That's likely an issue in current main, I'm AFK but if you're able to check that'd be great.

Don't think that's a problem been introduced here

@Steve-Mcl
Copy link
Contributor

Don't support dynamic label for ui-dropdown?
As in not documented right?

The use of null to clear it, which is what I thought is what you're saying was not working?

Yeah. I thought the point of making helper functions was to cement this common behaviour at the core. As such I raised it as an issue. forgive me if I am missing something here.

@joepavitt
Copy link
Collaborator Author

joepavitt commented Jul 31, 2024

Is the problem here that a message containing only ui_update settings is stored as the last message? I think that such messages should not be stored, as the ui_update data are stored in the state store.

This could work, but not in scope of this issue/PR

@joepavitt
Copy link
Collaborator Author

I thought the point of making helper functions was to cement this common behaviour at the core

It's not listed as a supported or new feature, so no, this has lot been added, but can be in a follow up PR/issue

@Steve-Mcl
Copy link
Contributor

I thought the point of making helper functions was to cement this common behaviour at the core

It's not listed as a supported or new feature, so no, this has lot been added, but can be in a follow up PR/issue

Are any of these changes breaking or will users experience differing behaviour when this is merged Joe? If yes, then I would personally recommend we get the behaviour nailed down in this PR before we release it into the wild.

@Steve-Mcl
Copy link
Contributor

ui-number-group

chrome_qTk75XCTdo

  1. Sending payload with matching value does not slect the matching button. After a refresh it does become selected.
  2. Sending an updated label correctly sets the label. Sending null correctly reverts the label to static value. However, if the DB is refreshed, sending null no longer reverts label to static value.
  3. Sending a new set of options with same/matching values does not keep the current selection.

demo flow

[{"id":"8c7ef62c4af9d424","type":"inject","z":"f80624abfd69b02e","name":"ui_update.label Numbers","props":[{"p":"ui_update.label","v":"Numbers","vt":"str"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1250,"y":80,"wires":[["75551956e485bac6"]]},{"id":"285f16e64f307827","type":"inject","z":"f80624abfd69b02e","name":"ui_update.label null","props":[{"p":"ui_update.label","v":"null","vt":"json"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1270,"y":120,"wires":[["75551956e485bac6"]]},{"id":"48af77c3c271917d","type":"inject","z":"f80624abfd69b02e","name":"ui_update.options numbers","props":[{"p":"ui_update.options","v":"[{\"value\":1,\"label\":\"One\",\"icon\":\"numeric-positive-1\"},{\"value\":true,\"label\":\"Two\",\"icon\":\"numeric-2-box\"},{\"value\":\"tree\",\"label\":\"T_ree\",\"icon\":\"tree\"}]","vt":"json"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1250,"y":200,"wires":[["75551956e485bac6"]]},{"id":"7a50a8231976b836","type":"inject","z":"f80624abfd69b02e","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"1","payloadType":"num","x":1310,"y":240,"wires":[["75551956e485bac6"]]},{"id":"17dc21e121c2184c","type":"inject","z":"f80624abfd69b02e","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"tree","payloadType":"str","x":1310,"y":320,"wires":[["75551956e485bac6"]]},{"id":"cb13084236dfcfc8","type":"inject","z":"f80624abfd69b02e","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"true","payloadType":"bool","x":1310,"y":280,"wires":[["75551956e485bac6"]]},{"id":"80798abd6bac3521","type":"debug","z":"f80624abfd69b02e","name":"debug 187","active":true,"tosidebar":true,"console":false,"tostatus":true,"complete":"payload","targetType":"msg","statusVal":"payload","statusType":"auto","x":1540,"y":200,"wires":[]},{"id":"8021da3ecf8e21f9","type":"inject","z":"f80624abfd69b02e","name":"ui_update.options null","props":[{"p":"ui_update.options","v":"null","vt":"json"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1260,"y":360,"wires":[["75551956e485bac6"]]},{"id":"2c1654b9f7489958","type":"ui-button-group","z":"f80624abfd69b02e","name":"","group":"5c9407416fba1d1b","order":1,"width":6,"height":1,"label":"stars","rounded":false,"useThemeColors":true,"passthru":false,"options":[{"label":"Cap'n A","icon":"star-circle-outline","value":"1","valueType":"num","color":"#009933"},{"label":"Crescent","icon":"star-crescent","value":"true","valueType":"bool","color":"#999999"},{"label":"David","icon":"star-david","value":"tree","valueType":"str","color":"#ff6666"}],"topic":"topic","topicType":"msg","x":1530,"y":140,"wires":[["80798abd6bac3521"]]},{"id":"75551956e485bac6","type":"junction","z":"f80624abfd69b02e","x":1440,"y":140,"wires":[["2c1654b9f7489958"]]},{"id":"5c9407416fba1d1b","type":"ui-group","name":"PR-1123","page":"868aaffa6da2e1ce","width":"6","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"868aaffa6da2e1ce","type":"ui-page","name":"PR-1123","ui":"22ea43815413e748","path":"/page13","icon":"home","layout":"grid","theme":"c2ff5ba1f92a0f0e","order":1,"className":"","visible":"true","disabled":"false"},{"id":"22ea43815413e748","type":"ui-base","name":"base","path":"/dashboard","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"navigationStyle":"default","titleBarStyle":"default"},{"id":"c2ff5ba1f92a0f0e","type":"ui-theme","name":"Default","colors":{"surface":"#ffffff","primary":"#0094ce","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"},"sizes":{"pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]

@joepavitt
Copy link
Collaborator Author

Are any of these changes breaking or will users experience differing behaviour when this is merged Joe?

There should be 0 difference to UX as a result of this PR

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Jul 31, 2024

Are any of these changes breaking or will users experience differing behaviour when this is merged Joe?

There should be 0 difference to UX as a result of this PR

Ok, well in which case my semi-thorough testing should probably be halted until I can establish a baseline from main to see if this PR is causing the issues I see or if they are existing issues.

Before I halt, I will post findings for UI-Button (rather than let my findings go to waste)

ui-button

chrome_7lXqVChiYj

  1. Setting dynamic label works. sending null for label reverts it. However, after a refresh, it can not be reverted anymore.
  2. Setting dynamic icon + iconPosition works. sending null for icon + iconPosition reverts them. However, after a refresh, they can no longer be reverted.

chrome_o65l3ZoRzM

  1. Setting dynamic label works. sending null for label reverts it. However if we refresh with the label reverted via a null, the button has no label after a refresh
  2. Setting dynamic icon + iconPosition works. sending null for icon + iconPosition reverts them. However, if we refresh with the icon + iconPosition revered via a null, the button is lost. An error in console reveals the issue is in makeMdiIcon
TypeError: Cannot read properties of null (reading 'replace')
    at Proxy.makeMdiIcon (index-BPCbQ1sF.js:47:36548)
    at Proxy.prependIcon (index-BPCbQ1sF.js:47:35741)
    at ReactiveEffect.fn (index-BPCbQ1sF.js:10:13076)
    at ReactiveEffect.run (index-BPCbQ1sF.js:10:1918)
    at get value (index-BPCbQ1sF.js:10:13345)
    at Object.get [as prependIcon] (index-BPCbQ1sF.js:17:34086)
    at Proxy._sfc_render$h (index-BPCbQ1sF.js:47:37151)
    at renderComponentRoot (index-BPCbQ1sF.js:17:7654)
    at ReactiveEffect.Ce [as fn] (index-BPCbQ1sF.js:17:54912)
    at ReactiveEffect.run (index-BPCbQ1sF.js:10:1918)

image

demo flow

[{"id":"a19fe7752981edcb","type":"inject","z":"f80624abfd69b02e","name":"icon & position left","props":[{"p":"ui_update.icon","v":"menu-left","vt":"str"},{"p":"ui_update.iconPosition","v":"left","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1170,"y":80,"wires":[["225e4e1efcbc0ddc"]]},{"id":"019c68b113848730","type":"inject","z":"f80624abfd69b02e","name":"icon & position null","props":[{"p":"ui_update.icon","v":"null","vt":"json"},{"p":"ui_update.iconPosition","v":"null","vt":"json"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1170,"y":120,"wires":[["225e4e1efcbc0ddc"]]},{"id":"303170327edd25aa","type":"inject","z":"f80624abfd69b02e","name":"ui_update.label BTN TWO","props":[{"p":"ui_update.label","v":"BTN TWO","vt":"str"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1150,"y":240,"wires":[["1e59de55be3906d2"]]},{"id":"835d0f44edce12c9","type":"inject","z":"f80624abfd69b02e","name":"ui_update.label null","props":[{"p":"ui_update.label","v":"null","vt":"json"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1170,"y":280,"wires":[["1e59de55be3906d2"]]},{"id":"97fc0235988ce987","type":"debug","z":"f80624abfd69b02e","name":"debug 188","active":true,"tosidebar":true,"console":false,"tostatus":true,"complete":"payload","targetType":"msg","statusVal":"payload","statusType":"auto","x":1440,"y":180,"wires":[]},{"id":"225e4e1efcbc0ddc","type":"ui-button","z":"f80624abfd69b02e","group":"5c9407416fba1d1b","name":"Button 1","label":"Button 1","order":1,"width":"3","height":"1","emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"menu-right","iconPosition":"right","payload":"1","payloadType":"num","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","x":1380,"y":100,"wires":[["97fc0235988ce987"]]},{"id":"1e59de55be3906d2","type":"ui-button","z":"f80624abfd69b02e","group":"5c9407416fba1d1b","name":"Button 2","label":"Button 2","order":2,"width":"3","height":"1","emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"","iconPosition":"right","payload":"two","payloadType":"str","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","x":1380,"y":260,"wires":[["97fc0235988ce987"]]},{"id":"5c9407416fba1d1b","type":"ui-group","name":"PR-1123","page":"868aaffa6da2e1ce","width":"6","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"868aaffa6da2e1ce","type":"ui-page","name":"PR-1123","ui":"22ea43815413e748","path":"/page13","icon":"home","layout":"grid","theme":"c2ff5ba1f92a0f0e","order":1,"className":"","visible":"true","disabled":"false"},{"id":"22ea43815413e748","type":"ui-base","name":"base","path":"/dashboard","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"navigationStyle":"default","titleBarStyle":"default"},{"id":"c2ff5ba1f92a0f0e","type":"ui-theme","name":"Default","colors":{"surface":"#ffffff","primary":"#0094ce","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"},"sizes":{"pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]


What I will state is, there is a common theme across many of the issues related to override/null/refresh issues

@arturv2000
Copy link
Contributor

Most of these issues exists in Main, regarding the null/reverting to original configuration issue was already talked about in this PR, and was decided to address that latter on.
#1123 (comment)

@arturv2000
Copy link
Contributor

arturv2000 commented Jul 31, 2024

Regarding the ui-number-group it is a bug from this PR

before was this:

const updates = msg.ui_update
if (typeof updates?.label !== 'undefined') {
   this.dynamic.label = updates.label
}
if (typeof updates?.options !== 'undefined') {
   this.dynamic.options = updates.options
}

And now is:

const updates = msg.ui_update
this.updateDynamicProperty('label', updates.label)
this.updateDynamicProperty('options', updates.options)

But when we inject a message via Node-Red, the onDynamicProperties is called, but the object ui_update may not exist...

Replacing with:

        onDynamicProperty (msg) {
            const updates = msg.ui_update
            if (!updates) {
                return
            }
            this.updateDynamicProperty('label', updates?.label)
            this.updateDynamicProperty('options', updates?.options)
        },

Solves the main issue (Not the null not returning to original configuration)

The same behaviour may be happening in other widgets.

The fact that is not selected upon a refresh, is because the last message is replayed (maybe this term is not technical correct), and if the last message was only to set the ui_update.options uppon refresh the widget received a new configuration but no msg.payload to force a selection.

@joepavitt
Copy link
Collaborator Author

joepavitt commented Aug 6, 2024

So, I'm fairly sure this is all good to go now. @Steve-Mcl the null concerns will be treated as a follow up feature request, and are not a blocker for this PR

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the newly introduced issues are indeed resolved.

Code review all good.

NOTE: This is approved in the knowledge that there are a few issues related to mostly to setting then reseting dynamic props but they are existing and are to be addressed separately.

In case they are not all spelled out in the issue, the ones I seen are:

  • dropdown

    • Applying dynamic label then resetting it with a null followed by a refresh causes the label to disappear
    • Applying dynamic options then resetting them with a null followed by a refresh causes the options to disappear
  • form

    • Applying dynamic label then resetting it with a null followed by a refresh causes the label to disappear
    • Applying dynamic options then resetting them with a null followed by a refresh causes the options to disappear
    • After Applying dynamic options, the first 2 fields appear to be bound (typing in one field will update the other)
  • button

    • Applying dynamic icon then resetting it with a null followed by a refresh causes the button to disappear
    • Applying dynamic label then resetting it with a null followed by a refresh causes the buttons label to disappear
  • button group

    • Applying dynamic label then resetting it with a null followed by a refresh causes the label to disappear
    • Applying dynamic options then resetting them with a null followed by a refresh causes the options to disappear

NOTE: This ↑ was not exhaustive - only a selection of the dynamic ops and widgets were tested for the purposes of this review.

@joepavitt
Copy link
Collaborator Author

Thanks Steve

@joepavitt joepavitt merged commit 1d19cf6 into main Aug 7, 2024
1 of 2 checks passed
@joepavitt joepavitt deleted the 1122-dynamic-props-rewrite branch August 7, 2024 14:00
@omrid01
Copy link

omrid01 commented Aug 14, 2024

So just to make sure, there is no backwards compatibility impact:

  • server node will continue to send 'on-widget-load' to new clients, with their respective data from the datastore?
  • Will on-widget-load now be sent also when the datastore has no data for this node?

@joepavitt
Copy link
Collaborator Author

server node will continue to send 'on-widget-load' to new clients, with their respective data from the datastore?

correct

Will on-widget-load now be sent also when the datastore has no data for this node?

Yes, correct

@omrid01
Copy link

omrid01 commented Aug 14, 2024

Excellent, thank you.
Is there a way to know the dash-2.0 version (programmatically from within the server node?)

@joepavitt
Copy link
Collaborator Author

Not that I can think of - any reason you'd need to know for this though, in your load handler, just account for if (!msg) being a ting, and it should be backward compatabile.

@omrid01
Copy link

omrid01 commented Aug 15, 2024

I'm trying to think ahead. Dash-2.0 is continuously evolving with new functionality, and it would be good to enable a node to know the dash-2.0 version it is running on (and determine which framework functionality is available). Equivalent to checking the NodeJS version to determine if a new JavaScript feature is available.

Specifically in my case, it's chicken & egg. I can easily check if a message is null or undefined once I receive the load notification, but will never get this notification in the first place if datastore was empty... :-) so knowing the version will enable me to retain the current workaround (forcing a dummy datastore entry) for users who upgrade the node but are still on an older dashboard version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment