Fix "DelayNode: set name: not writable!"

[phoddie]

Ha! The infamous "override mistake" strikes again. In the interest of strict Node-RED compatibility I think we could use Object.defineProperty (as the link explains).

[ralphwetzel]

@phoddie -- Looks like we need a better way to handle this infamous issue: I came across another node that thows the same exception, again w/ property name. I don't think it's a sustainable strategy if all those nodes have to be "fixed" to make them running. Any ideas?

[ralphwetzel]

https://github.com/phoddie/node-red-mcu/blob/5d8bde93c2b880942ed17dc990b0a5d8f6c4b87f/nodered.js#L217-L218

Wouldn't that be an option?

    if (name)
        properties.name = {value: name; writable: true};

[ralphwetzel]

Wouldn't that be an option?

It seems to be one step in the right direction - yet it's not enough: As the config provided by nodered2mcu to node.onStart has no property name, an assignment like

https://github.com/phoddie/node-red-mcu/blob/5d8bde93c2b880942ed17dc990b0a5d8f6c4b87f/nodes/delay/89-delay.js#L93

leads to this.name === undefined.

Thus a change in nodered2mcu is necessary as well...

[phoddie]

So, in fact, it isn't the override mistake in the original report. The error is the same, but here it is just the base Node class making the name property unwritable. That seems like a good thing -- there's no intent in Node-RED for a node to change its name at runtime. But, as you observed, some existing node implementations do set name directly (many do not). I want to look more carefully at the Node-RED implementation to see what's going on there before making a change.

[phoddie]

So.... RED.nodes.createNode() sets this.name. That the Delay node sets the name property too is redundant -- it is setting it to the value it already has. That's probably historical. In Node-RED it works because name is writable. In Node-RED MCU I made name and id read-only because that seems safer (and potentially allows for some other optimizations down the road).

There are some other core nodes that set name (Function, Debug, Template, TCP In, HTTP Proxy). These aren't a problem because Node-RED MCU doesn't use these implementations.

If name is not intended to be changeable at runtime, then we should just comment out the setting of name in the Delay node (as you did in your original PR!). We could propose to the Node-RED core to remove the unnecessary code (marginally smaller and faster). If name is intended to be changeable, then we should modify the Node class in Node-RED MCU to do that (by setting writable as you suggest above). The question of intent is for @knolleary, @dceejay, and @sammachin. Maybe one of them can help?

[ralphwetzel]

If name is not intended to be changeable at runtime, then we should just comment out the setting of name in the Delay node (as you did in your original PR!). We could propose to the Node-RED core to remove the unnecessary code (marginally smaller and faster).

I'm less concerned about the core nodes. As you said, we can get these "under control". The node I mentioned above yet is a contributed one. And there are - very likely - numerous others that show this pattern. I'd vote to find a solution that does not demand to change all those community nodes either.

[phoddie]

@ralphwetzel – I understand. I'm just trying be careful about efficiency and reliability along with compatibility. But compatibility is the key word here. For nodes that have not been adapted for the Node-RED MCU runtime, we have the CompatibilityNode. It implements several things that are sub-optimal to be compatible with the full Node-RED. We can do that here too. That allows nodes adapted for Node-RED MCU to remain optimal and others to remain compatible. I've committed a change to the CompatibilityNode that solves the problem you reported with the Delay node. Perhaps it will also work for your cases?

I'd still like to understand what is intended here by Node-RED.

[ralphwetzel]

@phoddie -- Sorry. I closed this PR to test your commit. Guess it's obsolete now anyway.

cb15aeb addresses the first part of the topic, the name: not writable! error. There's yet still the issue that this.name = n.name creates this.name === undefined.

What do you think about the following idea:

        #name
        [...]

        if (name) {
            this.#name = name;
            let properties = {
                name: { 
                    get: function() {return this.#name},
                    set: function() {}
                }
            }
            Object.defineProperties(this, properties);
        }

This is even a bit more effort & a bit more sub-optimal - and it solves the discussed topics.

[phoddie]

There's yet still the issue that this.name = n.name creates this.name === undefined

Yes, it does. Isn't that what happens in Node-RED if the name is undefined? If you don't like that, we can do this:

    constructor(id, flow, name, module) {
        super(id, flow, undefined);
        if (undefined !== name)
            this.name = name;
        this.#module = module;
    }

[ralphwetzel]

With reference to the DelayNode:

function DelayNode(n) {
    RED.nodes.createNode(this, n);
    [...]
    this.name = n.name;

nodered2mcu strips the name property from the node's config (n) - which is a design decision for optimization.

Thus n.name is undefined & this.name = n.name later overwrites the name property set accordingly in the constructor - with undefined.

To prevent this & keep the correct value for this.name, a) name needs to be either defined as property of the config (n) or b) we have to prevent that this.name becomes overwritten.

a) has an impact on the effort to optimize the resources & thus is no option. The idea I proposed earlier intends to implement b): Prevent that this.name can be overwritten with the "wrong" value.

Thinking this further, it might even be better to address exactly just this one special case:

[...]
    set: function(name) {
        if (name)
            this.#name = name;
    }

Why should all this effort be necessary? Because there're nodes that use their name when sending status or debug or other messages (e.g. MQTT). Hopefully this lengthy statement is clear enough to explain my thoughts...

[phoddie]

A private field is expensive, because it consumes a slot even when it has no assigned value. Adding a setter dynamically also takes memory (and the corresponding getter is required). An easier solution is to pass the name through. That's a trivial change in nodered2mcu:

const {type, id, /* name, */ ...c} = {...config};

The RAM used by the including the name in the configuration passed to the node is temporary, whereas the slots on the instance take RAM for the lifetime of the flow. Not having to use a getter/setter to access the property is faster too. (Note that this change should be used together with the latest version of the constructor above).

The best answer is still that the implementation of nodes don't set the name property. ;)

[sammachin]

HI @phoddie I'm not totally sure what the question is here but am I right in thinking that if a node has a property of name and that property changes at runtime this is causing you an issue?

As far as I'm aware there's nothing special about the name property, node-red does reserve various single char properies such as x y etc that are used internally but anything else should be fine for nodes to use.

Name is commonly used to give an instance of a node a specific label in the flow, and often the editor code will read this property to then set label which is used as the display on the editor instead of the nodes default eg Delay

I can't see anywhere that the delay node does anything particularly odd with the property it just gets read from the node config here https://github.com/node-red/node-red/blob/master/packages/node_modules/%40node-red/nodes/core/function/89-delay.js#L88

[ralphwetzel]

That's a trivial change in nodered2mcu:

This is the a) option. Perfect! 👍

[phoddie]

I believe that with the latest nodered2mcu tool from the Moddable SDK and Node-RED MCU runtime from this repository, that the problem is addressed: the name property is writable for nodes running under the Compatibility Node, but otherwise read-only (like id).