search

dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
329 stars 78 forks source link

Performance issue due to native modules being lower priority #44

Closed abemedia closed 1 year ago

abemedia commented 1 year ago

I have a custom source loader which loads modules from a remote source and upon updating this package spotted an issue when loading native modules.

Due to https://github.com/dop251/goja_nodejs/pull/36 it now tries to download the module from the remote source before attempting to load the native module and this log should highlight the issue with that:

JS script:

console.log('hello world')

Log output:

time="2023-01-21T13:00:26Z" level=info msg="downloading node:console..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:console.js..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:console.json..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:console/package.json..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:console/index.js..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:console/index.json..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:util..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:util.js..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:util.json..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:util/package.json..."
time="2023-01-21T13:00:26Z" level=info msg="downloading node:util/index.js..."
time="2023-01-21T13:00:27Z" level=info msg="downloading node:util/index.json..."
2023/01/21 13:00:27 hello world

It's making 12 HTTP requests before attempting to load the native module!

Possible workarounds could be either making the priority configurable through an option on require.NewRegistry, or introducing a sentinel error that tells the resolver to stop looking and move on to attempting to load the module from the native modules. My preference would be implementing both! Happy to submit a PR if this is something you'd merge.

dop251 commented 1 year ago

I've just realised the originally proposed fix in #36 was more correct. The 'core' module should be preferred, but the name of the module should not be normalised prior to the resolution.