chrisgoringe / cg-use-everywhere

Apache License 2.0
355 stars 28 forks source link

Strict undefined check in `is_UEnode` not handling null. #184

Closed rgthree closed 2 months ago

rgthree commented 2 months ago

Came across this. Sometimes node_or_nodeType.type is null (not undefined) which causes an issue in this code in use_everywhere_utilities.js.

// in use_everywhere_utilities.js
function is_UEnode(node_or_nodeType) {
    var title = node_or_nodeType.type;
    if (title===undefined) title = node_or_nodeType.comfyClass;
    if (title===undefined) return false;
    return (title.startsWith("Anything Everywhere") || title==="Seed Everywhere" || title==="Prompts Everywhere")
}

This method seems to run on construction of a ComfyNode where node_or_nodeType.type is always undefined always. However, a bunch of rgthree-nodes this is actually null and this breaks becuase of the title.startsWith. This isn't currently invoked but I was playing with the idea of better alignment with the default ComfyNode, which would allow this to run on these rgthree nodes.

I'm still trying to figure out why type is always undefined in ComfyNode and null in these rgthree nodes, but I would still consider this a bug as null could make it through to that startsWith call.

By looking at this, my recommendation would be to change the two title === undefined to tile == null (with two equals) which will match both undefined and null.

rgthree commented 2 months ago

FWIW, I took a look at why there's the difference between null and undefined with ComfyUI and some rgthree-nodes.

Turns out, LiteGraph sets type to null explicitly when it's instantiating a new LGraphNode, and rgthree-nodes extend LGraphNode directly and correctly.

Turns out, ComfyNodes don't actually extend LiteGraph's nodes, so never call into the LGraphNode constructor and, thus, type is left as undefined. If anything, this feels like a high-level buggy behavior in ComfyUI's ComfyNode for not actually running the LGraphNode constructor.

I don't feel like hacking or working around LiteGraph's LGraphNode constructor to reset to undefined, since leaving as-is feels like the right thing, LiteGraph-wise. What this means is if I do submit the change to allow other custom nodes to run nodeCreated for rgthree-nodes, this is_UEnode will throw an error for title being null. Nothing negative should happen, just errors in the console.

chrisgoringe commented 2 months ago

huh. Nice find. Cleaned it up like this to avoid the whole null/undefined thing completely:

function is_UEnode(node_or_nodeType) {
    const title = node_or_nodeType.type ?? node_or_nodeType.comfyClass;
    return ((title) && (title.startsWith("Anything Everywhere") || title==="Seed Everywhere" || title==="Prompts Everywhere"))
}

I never really got my head around javascript truthiness :)