phoddie / node-red-mcu

Node-RED for microcontrollers
120 stars 18 forks source link

Change Node outputs <str> but <num> #60

Closed ralphwetzel closed 1 year ago

ralphwetzel commented 1 year ago

Hi Peter! This is a very subtle one! 😉 The following flow ...

image

... with a change node of

image

... outputs "1" (typeof == string) but 1 (typeof == num). ... because nodered2mcu creates msg.payload = msg.payload.toString().replaceAll("test", 1); ... because replaceAll() method returns a new string.

There's another constraint to be considered: When you concatenate two of those rules...

image

... the second, despite no match, modifies as well msg.payload (!):

msg.payload = msg.payload.toString().replaceAll("test", 1);
msg.payload = msg.payload.toString().replaceAll("check", 0);

I haven't tested for further edge cases...

flow.json ```json [ { "id": "470371201148e1dd", "type": "tab", "label": "Flow 20", "disabled": false, "info": "", "env": [], "_mcu": { "mcu": false } }, { "id": "35f87b3c94fffd9e", "type": "inject", "z": "470371201148e1dd", "name": "", "props": [ { "p": "payload" } ], "repeat": "", "crontab": "", "once": false, "onceDelay": 0.1, "topic": "", "payload": "test", "payloadType": "str", "_mcu": { "mcu": false }, "x": 450, "y": 160, "wires": [ [ "ddb59a0430163f6e" ] ] }, { "id": "6b0877f2abaf85dc", "type": "debug", "z": "470371201148e1dd", "name": "debug 76", "active": true, "tosidebar": true, "console": false, "tostatus": true, "complete": "true", "targetType": "full", "statusVal": "payload", "statusType": "auto", "_mcu": { "mcu": false }, "x": 800, "y": 160, "wires": [] }, { "id": "ddb59a0430163f6e", "type": "change", "z": "470371201148e1dd", "name": " test -> 1", "rules": [ { "t": "change", "p": "payload", "pt": "msg", "from": "test", "fromt": "str", "to": "1", "tot": "num" } ], "action": "", "property": "", "from": "", "to": "", "reg": false, "_mcu": { "mcu": false }, "x": 620, "y": 160, "wires": [ [ "6b0877f2abaf85dc" ] ] } ] ```
phoddie commented 1 year ago

Subtle, indeed. Node-RED has type dependent rules for this! It should be possible, but it will take a little time. Here's the relevant bit from 15-change.js:

                                        } else if (rule.t === 'change') {
                                            current = RED.util.getMessageProperty(msg,property);
                                            if (typeof current === 'string') {
                                                if ((fromType === 'num' || fromType === 'bool' || fromType === 'str') && current === fromValue) {
                                                    // str representation of exact from number/boolean
                                                    // only replace if they match exactly
                                                    RED.util.setMessageProperty(msg,property,value);
                                                } else {
                                                    current = current.replace(fromRE,value);
                                                    RED.util.setMessageProperty(msg,property,current);
                                                }
                                            } else if ((typeof current === 'number' || current instanceof Number) && fromType === 'num') {
                                                if (current == Number(fromValue)) {
                                                    RED.util.setMessageProperty(msg,property,value);
                                                }
                                            } else if (typeof current === 'boolean' && fromType === 'bool') {
                                                if (current.toString() === fromValue) {
                                                    RED.util.setMessageProperty(msg,property,value);
                                                }
                                            }
phoddie commented 1 year ago

I think I've got this now emulating the quirky Node-RED behaviors. Both of your examples are working along with several others I tried. There could be edge cases of the edge cases.... we'll see. The changes are to both nodered2mcu and the Change node runtime (nodered.js) and should land tomorrow.

I can't imagine someone expecting the behavior Node-RED implements. It is very obscure. But, I suppose, someone might depend on it unintentionally, so we should try to do the same on the MCU.

(FWIW – for future reference, the flow for the first example was very helpful, thank you. It would have been nice to also have the flow for the second example as I got it wrong the first time...)

phoddie commented 1 year ago

Fixed! Maybe. ;)

Be sure to update nodered2mcu along with the Node-RED MCU Edition runtime before trying it.

phoddie commented 1 year ago

Closing this as the test cases are fixed. If there are additional issues, please open a new issue. Thank you.