[v18] `source` value is ignored from the loader's `load` function with `format: 'commonjs'`

devjiwonchoi commented 3 weeks ago

[!NOTE] This issue is limited to v18. Works fine on v20.

Version

Confirmed: v18.19.0, v18.20.3

Platform

Darwin pro-m3-2023-36gb.local 24.0.0 Darwin Kernel Version 24.0.0: Tue Sep 24 23:37:25 PDT 2024; root:xnu-11215.1.12~1/RELEASE_ARM64_T6030 arm64

`source` value from the loader's `load` function with `format: 'commonjs'` is ignored in v18.

Subsystem

No response

What steps will reproduce the bug?

Repro: https://github.com/devjiwonchoi/repro-nodejs-loader-cjs-source

The Node.js loader hook function load can return properties: format and source. When running on Node.js v18 with format set to commonjs, the source property is ignored and does not affect the loaded module.

// index.js
console.log('hello from index.js');

// loader.mjs
export async function load() {
  return {
    format: 'commonjs',
    shortCircuit: true,
    source: `console.log("hello from loader.mjs");`,
  }
}

// optional: register.js
const { register } = require('node:module')
const { pathToFileURL } = require('node:url')

register('./loader.mjs', pathToFileURL(__filename))

Run with `--import ./register.js`

node --import ./register.js ./index.js

Run with `--loader ./loader.mjs`

node --loader ./loader.mjs ./index.js

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

This issue is present on Node.js v18. Works fine on v20. (v19 doesn't support module.register API.)

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

The stdout must be:

{ format: 'module' }
hello from loader.mjs

What do you see instead?

The source is from the index.js, not modified from the loader.

{ format: 'commonjs' }
hello from index.js

Additional information

No response

RedYetiDev commented 3 weeks ago

Is this only on v18, or also on 20/22/23?

devjiwonchoi commented 3 weeks ago

Only on v18

aduh95 commented 3 weeks ago

It’s not exactly a bug, it’s a know limitation on Node.js 18.x because the loader hooks are running on the same thread, it cannot support CJS. That being said, the documentation is clearly wrong, definitely deserves fixing

devjiwonchoi commented 3 weeks ago

Is there any reference I can look at to learn the context of the limitation? P.S. I'm willing to contribute to the docs update!

aduh95 commented 1 week ago

I was misremembering things, the off-thread loaders have landed on 18.19.0. I guess the reason you're seeing that issue is that support for CJS source was simply never backported

devjiwonchoi commented 6 days ago

I'm aware that v18 will be EOL within a few months, but is there a chance it'd be back-ported to v18?

cc @RedYetiDev

nodejs / node