ERR_INVALID_RETURN_PROPERTY_VALUE when using module.register and module.registerHooks

[timokoessler]

Version

v23.9.0

Platform

Darwin MacBook-Pro 24.3.0 Darwin Kernel Version 24.3.0: Thu Jan  2 20:24:23 PST 2025; root:xnu-11215.81.4~3/RELEASE_ARM64_T8122 arm64

Subsystem

No response

What steps will reproduce the bug?

Create the following files:

instrument.js

import * as mod from "module";

mod.registerHooks({
  load(url, context, nextLoad) {
    return nextLoad(url, context);
  },
});

mod.register(new URL("hooks.js", import.meta.url).toString());

hooks.js

export async function load(url, context, nextLoad) {
  return nextLoad(url, context);
}

app.js

console.log("Hello from app.js");

Run node --import ./instrument.js ./app.js

How often does it reproduce? Is there a required condition?

Always, if the project includes a CJS file.

What is the expected behavior? Why is that the expected behavior?

No exception, logs Hello from app.js

What do you see instead?

node --import ./instrument.js ./app.js
node:internal/modules/customization_hooks:276
    throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
          ^

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected a string, an ArrayBuffer, or a TypedArray to be returned for the "source" from the "load" hook but got null.
    at validateLoad (node:internal/modules/customization_hooks:276:11)
    at nextStep (node:internal/modules/customization_hooks:190:14)
    at load (file:///Users/timokoessler/Git/nodejs-module-hooks-bug/instrument.js:5:12)
    at nextStep (node:internal/modules/customization_hooks:185:26)
    at loadWithHooks (node:internal/modules/customization_hooks:348:18)
    at #loadSync (node:internal/modules/esm/loader:790:14)
    at ModuleLoader.load (node:internal/modules/esm/loader:749:28)
    at ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:536:43)
    at #createModuleJob (node:internal/modules/esm/loader:560:36)
    at #getJobFromResolveResult (node:internal/modules/esm/loader:312:34) {
  code: 'ERR_INVALID_RETURN_PROPERTY_VALUE'
}

Additional information

Real world cases where this bug occurs

• Using a test runner that uses module.register (e.g. tapjs) with an app that uses module.registerHooks
• Using two instrumentation libraries, one using module.register (e.g. Sentry) and the other using module.registerHooks

More information

• The error does not occur when only ESM is used in the project
  • Simply create a package.json with "type:" "module" and re-run the command above.
• The error does not occur when only one of the register methods is used

Git Repo: timokoessler/nodejs-module-hooks-bug

[joyeecheung]

It looks like a quirk in module.register. If the hook looks like this:

export async function load(url, context, nextLoad) {
  const result = await nextLoad(url, context);
  process._rawDebug('in async hook', result);
  return result;
}

And it's the only hook used, it logs

in async hook [Object: null prototype] {
  format: 'commonjs',
  responseURL: 'file:///Users/joyee/projects/node/app.js',
  source: null
}

When loading CommonJS, the default load of the asynchronous hooks returns undefined as source, which means technically the returned result of the default loading step of async hooks cannot be returned as-is, because that violates the API requirement that source is mandatory. It's either a caveat or a bug in the CommonJS loading in module.register.

cc @nodejs/loaders

[JakobJingleheimer]

Sounds like it could be this caveat: https://nodejs.org/docs/latest/api/module.html#caveat-in-the-asynchronous-load-hook

[joyeecheung]

The source is manually reset to null here

node/lib/internal/modules/esm/load.js

Lines 121 to 125 in b6df128

	if (format === 'commonjs') {
	// For backward compatibility reasons, we need to discard the source in
	// order for the CJS loader to re-fetch it.
	source = null;
	}

I feel that in essence this is similar to #55808 and the proper way to fix it might be to just revert the approach in #47999 and always use the CJS loader to handle customization hooks in require(). And to keep module.register working, just re-implement module.register as an internal synchronous hook using module.registerHooks. Then require() goes through the synchronous hooks from the CJS loader which contains that module.register wrapper. Then we no longer need to have this "reset source to null and load it twice" quirk.

[joyeecheung]

Sounds like it could be this caveat

The docs seem to be talking about a different caveat (that is related to this one, or the cause of this one), because it seems to be talking about what happens when users return undefined in the asynchronous load hook. But the issue here is about the return value of the default nextLoad in the asyncrhonous hook. That means effectively an asynchornous load hook must always read the source from disk for CommonJS modules themselves. For example:

export async function load(url, context, nextLoad) {
  const result = await nextLoad(url, context);
  const { source } = result;
  const processedSource = source + '\nconsole.log("altered")';
  const final = { ...result, source: processedSource};
  process._rawDebug(final);
  return final;
}

Always logs this

{
  format: 'commonjs',
  responseURL: 'file:///Users/joyee/projects/node/app.js',
  source: 'null\nconsole.log("altered")'
}
altered

The concatnated source code never contains the original code of the module and always starts with null if it's a CommonJS module. All the asynchronous load hook must have this branch or something similar to load source code for CommonJS themselves in case the next hook in chain is the default step:

export async function load(url, context, nextLoad) {
  const result = await nextLoad(url, context);
  if (!result.source && context.format === 'commonjs') {
    result.source = fs.readFileSync(fileURLToPath(url)); // Load it yourself
  }
  result.source;  // Now it's safe to process source.
}

For example, this is done by import-in-the-middle https://github.com/nodejs/import-in-the-middle/blob/53a33a9b07799bff815864089a0c072d223df47b/lib/get-exports.js#L100

[joyeecheung]

FWIW, until the bug in module.register() gets fixed, I think to work around the issue you are facing, you could either add a hook to the end of chain to catch that case for module.register(), or ask the implementers of hooks that use module.register() to follow what import-in-the-middle does and be prepared that nextLoad does not always return the source if they use module.register() (as the documentation says, returning a potentially null source can be unsupported in the future anyway).

[timokoessler]

While testing the synchronous hooks with Sentry, I noticed that import-in-the-middle is also affected by this issue.
The fix applied in nodejs/import-in-the-middle#202 solves this issue, but causes two other cache related exceptions to be thrown in certain cases. This occurs for example, if require(esm) is used.

node:internal/modules/esm/translators:152
    return cjsCache.get(job.url).exports;
                                ^
TypeError: Cannot read properties of undefined (reading 'exports')

node:internal/modules/esm/module_job:325
      this.module.async = this.module.instantiateSync();
                                      ^
Error: request for 'file:///Users/timokoessler/Git/import-in-the-middle/lib/register.js' is not in cache

For the full stack trace and reproduction, see timokoessler/nodejs-module-hooks-bug/tree/main/cache-issues.

cc. @joyeecheung