ERR_INVALID_RETURN_PROPERTY_VALUE with load hook and commonjs

[anomiex]

Version

v20.9.0

Platform

Linux abulia 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1 (2023-10-07) x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

1. Create the following files:

   register-hooks.mjs

   import { register } from 'node:module';
   
   register( './hooks.mjs', import.meta.url );
   

   hooks.mjs

   import { readFileSync } from 'node:fs';
   
   export async function load(url, context, nextLoad) {
       if ( context.format !== 'commonjs' && context.format !== 'module' ) {
           return nextLoad(url, context);
       }
   
       // This might in practice be a check for `node_modules`.
       if ( url.endsWith( '/bar.cjs' ) ) {
           return nextLoad(url, context);
       }
   
       // Do some transformation of the source here.
       const source = readFileSync(new URL(url));
   
       return {
           format: context.format,
           source,
           shortCircuit: true,
       };
   } 
   

   foo.cjs

   const bar = require( './bar.cjs' );
   
   bar.hello();
   

   bar.cjs

   function hello() {
       console.log( "hello, world" );
   }
   
   module.exports = {
       hello
   };
   

2. Run node --import ./register-hooks.mjs foo.cjs

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

Always.

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

Script runs, printing "hello, world".

Because that's what this very simple code does. 🤷

What do you see instead?

node:internal/errors:497
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected array buffer, or typed array to be returned for the "source" from the "transformSource" function but got null.
    at new NodeError (node:internal/errors:406:5)
    at assertBufferSource (node:internal/modules/esm/translators:84:9)
    at stringify (node:internal/modules/esm/translators:94:3)
    at createCJSModuleWrap (node:internal/modules/esm/translators:219:12)
    at ModuleLoader.<anonymous> (node:internal/modules/esm/translators:267:10)
    at callTranslator (node:internal/modules/esm/loader:273:14)
    at ModuleLoader.<anonymous> (node:internal/modules/esm/loader:277:24)
    at new ModuleJob (node:internal/modules/esm/module_job:65:26)
    at #createModuleJob (node:internal/modules/esm/loader:290:17)
    at ModuleLoader.getJobFromResolveResult (node:internal/modules/esm/loader:248:34) {
  code: 'ERR_INVALID_RETURN_PROPERTY_VALUE'
}

Node.js v20.9.0

Additional information

This seems to have been introduced in v20.6.0, as v20.5.1 is not affected. Likely it's part of the enhancements to the load hook added in that version.

What seems to be happening is that once one commonjs file returns a non-null source, any sub-requires of that file will fail if null is returned for source despite that being the default behavior for the passed-in nextLoad function.

I ran into this while trying to figure out why playwright started giving me this error when I tried to update from Node 18 to Node 20. The loader can be found here, and the bug was being triggered by a mjs config file that imports a cjs file that requires something out of node_modules. They've also got some other stuff going on in there, if I hack that loader to always fill in source then it winds up hanging eventually at this line (it seems that sends a message on the MessagePort that the other side never reacts to?).

Considering the comment I see in the doc that "This behavior for nullish source is temporary — in the future, nullish source will not be supported", I suppose you might decide this isn't a bug, saying that any loader that "opts in" to this behavior should carry it all the way through.

[aduh95]

Before 20.6.0, the source value was completely ignored when format was 'commonjs'. As of 20.6.0, this is no longer the case, but one of the trade offs is that you need to supply the source for all the CJS modules. I agree it’s unfortunate that the default load has to return null for backward compatibility; if someone had an idea how to improve the DX, please share.

/cc @nodejs/loaders

[anomiex]

but one of the trade offs is that you need to supply the source for all the CJS modules.

This statement does not seem to match the documentation for 20.x.

In the column for "Acceptable types for source returned by load" for "commonjs" it says

{ string, ArrayBuffer, TypedArray, null, undefined }

There's also extended discussion below about the behavior:

Omitting vs providing a source for 'commonjs' has very different effects:

• When a source is provided, all require calls from this module will be processed by the ESM loader with registered resolve and load hooks; all require.resolve calls from this module will be processed by the ESM loader with registered resolve hooks; only a subset of the CommonJS API will be available (e.g. no require.extensions, no require.cache, no require.resolve.paths) and monkey-patching on the CommonJS module loader will not apply.
• If source is undefined or null, it will be handled by the CommonJS module loader and require/require.resolve calls will not go through the registered hooks. This behavior for nullish source is temporary — in the future, nullish source will not be supported.

If nothing else, perhaps it should mention in there that there's no mixing-and-matching, a load hook must return null or undefined for all 'commonjs' modules or for none of them? And possibly explicitly call out that in the latter case nextLoad's return value needs to have a source filled in?

[GeoffreyBooth]

If nothing else, perhaps it should mention in there that there’s no mixing-and-matching, a load hook must return null or undefined for all 'commonjs' modules or for none of them? And possibly explicitly call out that in the latter case nextLoad‘s return value needs to have a source filled in?

As a stopgap, yes, the docs should at least document the current state of affairs until we can improve it.

[mfucci-medable]

I am running into the same issue for the same reasons: Playwright config and upgrading to node v20.9
The app is using esm app and the Playwright config references a commonjs common config.

Per this thread, I am unclear what is the suggested stopgap:

• don't use node past v20.5.1?
• upgrading Playwright? (nope, some issue with latest version)
• changing esm to commonjs or the opposite? (I can't do that, this is shared code...)

=> I fail to see how updating the doc is a stopgap for the issue unless I don't understand something.

I am not clear as well if this is a bug or not that needs to be addressed in Playwright, or if by any chance I can modify something in my code..

Right now, downgrading node seems to be my only option...

[mfucci-medable]

Nevermind: I turned the common config into a hybrid ESM / CommonJs lib and now the problem above is resolved for the ESM app and the common config is still working for all other CommonJs apps...
=> seems like the issue mentioned here only happens when ESM and CommonJs is mixed

[anomiex]

=> seems like the issue mentioned here only happens when ESM and CommonJs is mixed

The issue happens if the load hook returns a non-null/undefined source for one commonjs file, which then requires another commonjs file for which the load hook tries to return a null/undefined source.

Playwright's current loader does this when the first commonjs file is not inside node_modules and the second is.

[lizthegrey]

Thanks for the minimal repro, I was wondering why Yarn PnP chained to Datadog IITM (wrapped by OTel-JS) was not working correctly post Node 20.6.

[lizthegrey]

Appears this will be fixed in 20.11.0 per #51124 and fbdf291 making it into the 20.11 candidate

[lizthegrey]

Confirmed fixed by the backport. 20.11 works flawlessly now with yarn pnp and with datadog iitm