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.