Adding support for installing ESM module nodes

Core Development
1

Adding support for installing ESM module nodes

I am working on fixing node-red to not fail when attempting to install a node written as an ESM module, e.g.

19 Sep 18:59:53 - [info] Server now running at http://127.0.0.1:1880/
19 Sep 18:59:53 - [info] Starting flows
19 Sep 18:59:53 - [info] Started flows
19 Sep 19:01:05 - [info] Installing module: @hlovdal/node-red-lowercase-in-typescript, version: 1.0.0
19 Sep 19:01:12 - [info] Installed module: @hlovdal/node-red-lowercase-in-typescript
19 Sep 19:01:12 - [info] Added node types:
19 Sep 19:01:12 - [info]  - @hlovdal/node-red-lowercase-in-typescript:lower-case : Error [ERR_REQUIRE_ESM]: require() of ES Module .../.node-red/node_modules/@hlovdal/node-red-lowercase-in-typescript/src/lower-case.js from .../node-red/packages/node_modules/@node-red/registry/lib/loader.js not supported.
Instead change the require of lower-case.js in .../node-red/packages/node_modules/@node-red/registry/lib/loader.js to a dynamic import() which is available in all CommonJS modules.

Tested with 3.1.0 (and master branch as of writing this (although not that many differences)).

I have a solution and I am ready to create a pull request. The contributing guidelines strongly suggest to discuss here first.

The fix itself is rather simple, just using a dynamic import() instead of require.

But one question I have is regarding the content of the pull request: while working on this I have found and fixed a few other minor, unrelated issues like removing unused variables/functions. Can I include those in the same pull request or should I split the branch and make one pull request that purely fixes ESM support and another one with minor code improvements?

3 Likes
2

It would certainly make it easier if you could split them out.

Please also target the dev branch initially - this is for new feature development which this would be.

1 Like
3

Ok, I'll split the branch into two PRs.

1 Like
4

Though dynamic imports are async, so how is that being handled?

Be fantastic if this could be solved in a robust way.

5

The node loading code inside the runtime is already async, even if the current call to require is sync. So this switch is actually pretty transparent to everything else.

1 Like
6

Nice, good to hear. Afraid I've not had a chance to look at that part of the code. I'll watch out for the PR. This is something I've been hoping someone could solve a while.

I need to do some more refactoring of some of my code in readiness of future moves in this direction.

Already starting to work on a new module so as to move all the file system handling to a single promises-only based class and to remove the dependency on the fs-extra library. But it is quite a bit of work and probably wont be complete for quite a while.

7

I noticed that one of the CI builds failed:

> Use Node.js 16

Run actions/setup-node@v3
Found in cache @ /opt/hostedtoolcache/node/16.20.2/x64
Environment details

...
> Run tests
...
  2450 passing (2m)
  59 pending
  1 failing

  1) function node
       init function
         should delay handling messages until init completes:
     Error: Message received before init completed - was 191 expected >300
      at Object._inputCallback (test/nodes/core/function/10-function_spec.js:1738:34)
      at /home/runner/work/node-red/node-red/packages/node_modules/@node-red/runtime/lib/nodes/Node.js:76:33
      at Object.trigger (packages/node_modules/@node-red/util/lib/hooks.js:44:1839)
      at Object.Node._emitInput (packages/node_modules/@node-red/runtime/lib/nodes/Node.js:75:30)
      at Object.Node.emit (packages/node_modules/@node-red/runtime/lib/nodes/Node.js:70:240)
      at Object.Node.receive (packages/node_modules/@node-red/runtime/lib/nodes/Node.js:122:395)
      at deliverMessageToDestination (packages/node_modules/@node-red/runtime/lib/flows/Flow.js:164:1119)
      at Immediate._onImmediate (packages/node_modules/@node-red/runtime/lib/flows/Flow.js:165:466)
      at processImmediate (node:internal/timers:466:21)



Warning: Task "simplemocha:all" failed.a Use --force to continue.

Aborted due to warnings.

I am not able to reproduce this locally, installing nodejs 16.20.2 with nvm.

But looking into the stacktrace there seems to be something fishy going on because line 76 of Node.js is in the middle of a comment on my branch (with no changes of this file compared to dev):

$ cat -n packages/node_modules/@node-red/runtime/lib/nodes/Node.js | sed -n 70,80p 
    70
    71  util.inherits(Node, EventEmitter);
    72
    73  /**
    74   * Update the wiring configuration for this node.
    75   *
    76   * We try to optimise the message handling path. To do this there are three
    77   * cases to consider:
    78   *  1. this node is wired to nothing. In this case we replace node.send with a
    79   *     NO-OP function.
    80   *  2. this node is wired to one other node. In this case we set `this._wire`
$

Line 76 has an offset of one line compared to the master branch, so any stale checkout from there is not the cause either...

Any ideas?

8

The error is occurring here :point_up:

What is on that line in the test?

It certainly looks like a timing error

should delay handling messages until init completes

9 
$ cat -n test/nodes/core/function/10-function_spec.js | sed -n 1738p
  1738                              done(new Error(`Message received before init completed - was ${msg.delta} expected >300`))
$

So at least that makes sense with regards to line numbers.

10

The test is the following.

var functionNode = require("nr-test-utils").require("@node-red/nodes/core/function/10-function.js");
...
var helper = require("node-red-node-test-helper");
...
        it('should delay handling messages until init completes', function(done) {
            var flow = [{id:"n1",type:"function",wires:[["n2"]],initialize: `
                return new Promise((resolve,reject) => {
                    setTimeout(resolve,200)
                })`,
                func:"return msg;"
            },
            {id:"n2", type:"helper"}];
            helper.load(functionNode, flow, function() {
                var n1 = helper.getNode("n1");
                var n2 = helper.getNode("n2");
                var receivedMsgs = [];
                n2.on("input", function(msg) {
                    msg.delta = Date.now() - msg.payload;
                    receivedMsgs.push(msg)
                    if (receivedMsgs.length === 5) {
                        var errors = receivedMsgs.filter(msg => msg.delta < 200)
                        if (errors.length > 0) {
                            done(new Error(`Message received before init completed - was ${msg.delta} expected >300`))
                        } else {
                            done();
                        }
                    }
                });
                for (var i=0;i<5;i++) {
                    n1.receive({payload: Date.now(),topic: "msg"+i});
                }
            });
        });

So the threshold is 200ms and not 300 as the error indicates (and 191 is not that far off from that).

I pushed a new commit to trigger a re-rest.

By editing Gruntfile.js to only test this file

         simplemocha: {
            ...
            all: { src: ["test/nodes/core/function/10-function_spec.js"]},
            core: { src: ["test/nodes/core/function/10-function_spec.js"]},
            nodes: { src: ["test/nodes/core/function/10-function_spec.js"]},
        },

and then running several times I was able to reproduce once:

  1) function node
       init function
         should delay handling messages until init completes:
     Error: Message received before init completed - was 199 expected >300
      at Object._inputCallback (test/nodes/core/function/10-function_spec.js:1738:34)
11

I did some overnight testing and the test failure is an existing problem that applies to both dev and master. The test fails around 7% of the time. Details in github comment.

12

This topic was automatically closed 60 days after the last reply. New replies are no longer allowed.