Supporting typedInputs and simplified dynamic property setup

[Steve-Mcl]

Description

This is not tied directly to any one ISSUE.

This was based on recent forum feedback and my desire to support TypedInputs throughout DB2 widgets.

The changes implemented are opinionated and may not all may be welcomed but since I had some playtime, and I like where it got to, I thought I would raise a PR for feedback.

At present, this PR includes no doc updates or tests because if the changes are rejected I wont have wasted any time ;)

Whats changed

• Support DynamicProperties
// as part of registration of a node... const dynamicProperties = { label: true, mode: true, clearable: true, icon: true, iconPosition: true, iconInnerPosition: true } group.register(node, config, evts, { dynamicProperties })

  • This almost always removed the need for a node to override beforeSend
    • If a node does still want to use deynamic properties and hook the beforeSend they can call msg = await applyUpdates(RED, node, msg) to perform the same thing ui_base does.

• Support TypedInput

  • What: This PR introduces a pattern for supporting typed inputs for the payload.
    // as part of registration instead const typedInputs = { label: { nodeProperty: 'label', nodePropertyType: 'labelType' } } group.register(node, config, evts, { typedInputs })
  • Why: Because it is super annoying adding change nodes everywhere!
    • All nodes that accept input to have a typedInput #92
  • How: It works by evaluating node config property/propertyType and applying the computed value to the existing ui_update methods of transmitting the value to the client side.
  • NODES - For demonstrating the pattern, I have updated a few nodes to support TypedInput and utilise the simplified dynamic properties
    • NODE - ui-notification: Added TypedInput for the "message". This is achieved by including an object of { "message": { "property": "message", "propertyType": "messageType" } } in the node config and passing it to the node register method.
    • NODE - ui-text: Added TypedInput support for the label
    • NODE - ui-text-input: Added TypedInput support for the label and value
    • REF Programmatic (Mustache) Titles & Labels #555

• Notification specific improvements

  • Inhibit default passthru on notification
    • Why: Currently, the notification node sends multiple messages - 1 every time it gets and input msg and 1 when it closes.
    • What: This PR only sends the final msg upon timeout/dismiss/confirm
    • How: This is done by setting the config prop to false (hard coded)
  • Adds reason to msg._event
    • msg._event.reason will contain the relevant close reason of timeout, dismiss_clicked, or confirm_clicked
  • Only sends msg to Node-RED if show is true
    • Why: I could quite easily get notification node to emit both "confirm" or "dismiss" AND "timeout" (when clicking a button near the end of progress bar)

Demo

Related Issue(s)

Checklist

• I have read the contribution guidelines
• Suitable unit/system level tests have been added and they pass
• Documentation has been updated
  • Upgrade instructions
  • Configuration details
  • Concepts
• Changes flowforge.yml?
  • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
  • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

• Includes a DB migration? -> add the area:migration label

[Steve-Mcl]

• Support TypedInput
  • What: This PR introduces a pattern for supporting typed inputs for the payload.
  • Why: Because it is super annoying adding change nodes everywhere!
    • All nodes that accept input to have a typedInput #92
  • How: It works by evaluating node config property/propertyType and adding a temporary prop ui_payload to the msg that is used client side as the value. This should work for other widgets too.
  • NOTE: Adding JSONata might be an alt solution to the whole "support mustache for label / text / title / etc" debate
    • Programmatic (Mustache) Titles & Labels #555
    • which makes me think the temp ui_payload should probably be an object like ui_computed so that we can do the custom label stuff too e.g. { ui_computed: { payload: "evaluated payload", "label": "evaluated label" }

Another avenue may be to utilise the existing mechanism of ui_update and add the evaluated payload into that object 🤔

[bartbutenaers]

@Steve-Mcl,
My few cents about your proposal, for what it is worth:

1. I really like the idea of using Jsonata to mimic the Mustache expressions from dashboard D1. It is very powerful and then indeed you don't have to pollute your flows with Template nodes. Cool idea!

2. Personally I am not a fan to move the current ouput msg.payload to msg.ui_reason (in order to be able to keep the original input msg.payload). I know that it is advised that nodes should reuse and manipulate existing input messages (by adding data to it), instead of creating new output messages from scratch. In order to keep the (important) info from the input message. However the state of a ui nodes is mostly build upon multiple input messages. The last message might only contain some (unimportant) ui_update stuff, so not really to reuse such messages. Or do you mean that you reuse the last message stored in the data store perhaps? Because most ui nodes currently only store messages which have a payload, it might indeed be useful to reuse that one...

   Moreover when you use msg.ui_reason your flow again needs extra nodes. Because currently I can attach any kind of node to the output of my notification node, because the payload contains all the required info. When you start using msg.ui_reason I need to add Change nodes to move that reason into the payload again...

3. Only sends msg to Node-RED if show is true, makes sense to me. But could it perhaps be solved by moving the
   clear timer and interval code snippet BEFORE the socket.emit (because the emitting perhaps takes a bit too long)?

4. The default passthrough of messages is indeed something that was confusing for me while implementing my PR for the notification node. So indeed I would disable it.

[Steve-Mcl]

2. Moreover when you use msg.ui_reason your flow again needs extra nodes. Because currently I can attach any kind of node to the output of my notification node, because the payload contains all the required info. When you start using msg.ui_reason I need to add Change nodes to move that reason into the payload again

The current implementation of the UI node only sends "timeout" or "confirm" etc. What I did here was to move the reason for it closing into ui_reason so that the original payload could stay in payload and therefore be passed back for downstream use. And yes the msg passed back will be the last message sent in to that particular notification node.

The point of ui_reason is purely for routing the output message after it has been dismissed.

Fwiw, I don't mind what it's called. I chose ui______ something to minimise clashes with natural props on the msg.

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

E.g output 1-Success, output 2-cancelled etc

[Steve-Mcl]

3. Only sends msg to Node-RED if show is true, makes sense to me. But could it perhaps be solved by moving the
clear timer and interval code snippet BEFORE the socket.emit (because the emitting perhaps takes a bit too long)?

I've been bitten by timeouts in the past.

I did also move the clear calls above the emit but i could still cause dual messages. The interlock was a final catch all that stopped it happening.

[joepavitt]

I need to spend time on this before committing either way, and give it the attention it deserves, will quickly comment on:

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

[Steve-Mcl]

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

There would be no need to join. The msg (and its original payload) would be sent to the specific output - zero joins, zero switches - if it comes out of port 1 - success, port 2 - cancelled, etc

[colinl]

I need to spend time on this before committing either way, and give it the attention it deserves, will quickly comment on:

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

I think the confusion is that early on there was a suggestion to move just the result to a separate output, but I think that has been superseded by the suggestion of sending just to one output of the three, dependent on what is clicked. Effectively including a Switch node inside the Notification node.

[colinl]

Does this PR already include adding a note to the docs and help text to say that empty or 0 timeout disables the timeout?

[Steve-Mcl]

Does this PR already include adding a note to the docs and help text to say that empty or 0 timeout disables the timeout?

No Colin. No docs done at all - very much PoC at the moment.

Will make a mental note to include that kind of info if/when it evolves into something.

[colinl]

In fact that is actually just something missing from the docs for the existing node, it isn't anything to do with the suggested enhancements.

[Steve-Mcl]

@joepavitt as discussed recently. This rather large PR (sorry) covers the whole boilerplate setup required for nodes when using typedInputs and dynamic properties.

This to check (not sure i have handled, but my flows are now corrupted by the new features (for want of better words)

• When running this against flows developed on <v1.17.0, ensure typedInputs have a sane default and dont change anything operationally for the user.

I am more than happy to do a 1:1 when you come to review as I realise this is rather large.

[Steve-Mcl]

for consideration

[joepavitt]

@Steve-Mcl merging #1343 has triggered a merge conflict - you okay to sanity check it please?

[Steve-Mcl]

@Steve-Mcl merging #1343 has triggered a merge conflict - you okay to sanity check it please?

All done. Found and fixed an issue too so e2e tests should pass now.
Also, broke then fixed unit tests - bah!

[Steve-Mcl]

so this (rather large) piece of work now has conflicts again - bah!

There are some things in this PR that fix things like breaking of widgets after being dynamically updated etc - we should get this is. It is likely going to conflict with #1401 / #1428 and #30 / #1369

With these 3 large PRs hanging around, I am loathe to start some of the other tasks that will probably compound this further.

[joepavitt]

Thanks Steve, it's a huge PR which is going to take a lot of my time to review properly. I can't promise when I can get to it as there are a lot of fundamental changes here.

[Steve-Mcl]

I understand Joe.

This may be one that is simply pulled and tested.

Happy to do on a call tomorrow to make this swift(er)

[joepavitt]

Happy to do on a call tomorrow to make this swift(er)

Let's do that - mind putting something in that suits you please?