node-red / node-red

Low-code programming for event-driven applications
http://nodered.org
Apache License 2.0
18.93k stars 3.31k forks source link

Refactoring the `RED.nodes.import()` function #4757

Open GogoVega opened 2 weeks ago

GogoVega commented 2 weeks ago

Proposed changes

Refactoring the importNodes function.

Resolves also:

Other functions modified:

Checklist

knolleary commented 2 weeks ago

This function is so central to the editor, I am wary of anyone else doing a large refactoring of it. It contains 10 years of built up logic, and in many cases there are good reasons for the order things are done in.

We are also trying to get 4.0 shipped. I simply don't have bandwidth to give this work the attention it needs any time soon given there is no compelling reason to do it.

I will try to take a look at some point, but I can't say when.

GogoVega commented 1 week ago

No worries, focus on v4 and for my part I'll continue here - It's going to take me a while, then test it thoroughly.

Given all that this involves, I had not planned to bring it in for v4. So come back here as soon as you can but without pressure. Thanks 😉

GogoVega commented 2 days ago

Do we still have to handle the import of singleton config node? If so, the only currently unhandled case would be that multiple nodes use the same config node - this config node will need to be copied and re-associated with each node.

knolleary commented 8 hours ago

To be absolutely direct - as I was in an earlier comment - I'm not sure how to move forward with this PR. It is massive and changes such an important part of the editor code base. The PR not only moves code around, but also changes the logic in different places due to the various fixes you've identified. So not only do I have to understand how you've moved the code around, I need to understand why you've changed any particular bits of logic - which could be a bug you've fixed or a bug you've introduced.

I simply don't know when I will have the time needed to focus on this.