Hooks loading commonjs files uses import condition for require()

[privatenumber]

Version

21.5.0

Platform

macOS

Subsystem

No response

What steps will reproduce the bug?

Reproduction: https://github.com/privatenumber/bug-node-hooks-cjs-conditions

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

A Hook needs to be registered to load commonjs type files.

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

For the conditions to include require when require(file) is executed.

What do you see instead?

The import conditions only include import.

Here are the logs:

resolve: {
	"url": "file:///repro/src/index.cjs",
	"context": {
		"conditions": [
			"node",
			"import",
			"node-addons"
		],
		"importAttributes": {}
	}
}
load: {
	"url": "file:///repro/src/index.cjs",
	"context": {
		"format": "commonjs",
		"importAttributes": {}
	}
}
resolve: {
	"url": "file:///repro/src/file.cjs",
	"context": {
		"conditions": [
			"node",
			"import",
			"node-addons"
		],
		"importAttributes": {},
		"parentURL": "file:///repro/src/index.cjs"
	}
}
load: {
	"url": "file:///repro/src/file.cjs",
	"context": {
		"format": "commonjs",
		"importAttributes": {}
	}
}

Additional information

No response

[aduh95]

Consider sending minimal repros, i.e. no GitHub repo, and without unrelated details – this issue is only related to the context.conditions of the resolve hook, so it should be limited to that.

[aduh95]

Minimal repro:

"use strict";
require("node:module").register(
  `data:text/javascript,import assert from 'node:assert';export ${encodeURIComponent(
    function resolve(s, c, n) {
      if (s === "node:target") {
        assert.ok(
          !c.conditions.includes("import"),
          `Conditions should not include "import", actual ${JSON.stringify(
            c.conditions,
          )})`,
        );
        assert.ok(
          c.conditions.includes("require"),
          `Conditions should include "require", actual ${JSON.stringify(
            c.conditions,
          )})`,
        );
      }
      return n(s, c);
    },
  )}export ${encodeURIComponent(function load(u, c, n) {
    if (u === 'custom:cjs') {
      return {
        format: "commonjs",
        source: 'console.log(require.resolve("node:target"))',
        shortCircuit: true,
      };
    }
    return n(u, c);
  })}`,
);
import('custom:cjs').then(console.log, console.error);

/cc @nodejs/loaders

[privatenumber]

Thanks for validating the bug and providing a more minimal reproduction.

(For context, I personally find cloning a repo and running it to be more streamlined than manually recreating an environment. Also, I included logging to a file because the loader process wasn't logging everything to stdout.)

[aduh95]

For context, I personally find cloning a repo and running it to be more streamlined than manually recreating an environment.

From the point of view of the folks who triage the issue, it's much much quicker to have a single JS snippet to read, rather than a a repo to clone. Also, not using a repo will likely force the reporter to think on how to keep the repro as minimal as possible, which is going to come in handy when someone will need to write a test for that.

[targos]

Also a repo or something else to download is more difficult to review for malware / malicious code.

[EricMCornelius]

Does this also mean that require and import dependencies are both going to be cached as modules when using the node:module register hook?

Trying to track down a very baffling bug while trying to migrate from the deprecated --experimental-loader implementation to the new register() hooks - I've got a dependency which is both imported and required by other modules in my dependency stack - and all the attempted CJS requires are returning empty objects.

Haven't had success boiling it down to a minimal reproduction yet.

[saltman424]

@EricMCornelius Not sure if this applies to your use case, but I just saw this warning in the documentation:

Warning: The ESM load hook and namespaced exports from CommonJS modules are incompatible. Attempting to use them together will result in an empty object from the import. This may be addressed in the future.

[dustinlacewell]

This issue is making it harder and harder to get by in a universe with the presence of both monorepos and esm modules increasing all the time.