make experimental-specifier-resolution=node support extension-less files

[lachrist]

What is the problem this feature will solve?

I thought that the --experimental-specifier-resolution=node CLI option was made to emulate legacy cjs resolution algorithm. However the legacy cjs module loader can handle extension-less files whereas which is not the case with this CLI option:

> cat yo
console.log("yo");
> node yo
yo
> node --experimental-specifier-resolution=node yo
(node:33081) ExperimentalWarning: The Node.js specifier resolution in ESM is experimental.
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "file:///Users/soft/Desktop/workspace/appmap-agent-js/yo" 
    at new NodeError (node:internal/errors:371:5)
    at ESMLoader.load (node:internal/modules/esm/loader:380:13)
    at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:280:47)
    at async link (node:internal/modules/esm/module_job:70:21) {
  code: 'ERR_INVALID_MODULE_SPECIFIER'
}

Node.js v17.3.0

These extension-less files are actually common in the bin directory of many popular projects -- eg: mocha. This makes it hard to use --experimental-loader on these projects because the below command fails for the same reason as above:

NODE_OPTIONS="--experimental-loader=./loader.mjs --experimental-specifier-resolution=node" npx mocha

What is the feature you are proposing to solve the problem?

Consider extension-less files as commonjs modules as the legacy cjs loader does. This is can done be done easily by modifying legacyExtensionFormatMap in lib/internal/modules/esm/get_format.js:

const legacyExtensionFormatMap = {
  '__proto__': null,
  '.cjs': 'commonjs',
  '.js': 'commonjs',
  '.json': 'commonjs',
  '.mjs': 'module',
  '.node': 'commonjs',
  '': 'commonjs' // Added line
};

Is there a good reason why this has not been considered?

What alternatives have you considered?

1. Resolve the module in the loader provided by --experimental-loader and do not pass it to the default loader. This is a bazooka solution because the loader API basically enables to write one's own module resolution algorithm. We don't want to emulate the entire module resolution algorithm for such a minimal change.
2. Rename and change the target of the symbolic link installed by npm to add a .js extension. This must happen in a preloaded module. It is extremely hacky and does not work on windows.

[DerekNonGeneric]

@lachrist, thanks for opening this issue. This was implemented before my time, but wonder if this may actually be a regression or what seeing as how one of the main reasons for having this flag in the first place was to support Node's original resolution algorithm for extensionless files in ESM contexts according to our documentation for that CLI flag.

Sets the resolution algorithm for resolving ES module specifiers. Valid options are explicit and node.

The default is explicit, which requires providing the full path to a module. The node mode enables support for optional file extensions and the ability to import a directory that has an index file.
— https://nodejs.org/api/cli.html#--experimental-specifier-resolutionmode

/cc @nodejs/modules @nodejs/loaders

[DerekNonGeneric]

This was implemented before my time, but wonder if this may actually be a regression […]

The original commit that implemented this flag is b1094db, which show that this is not a regression, but has been this way the whole time. We may need to discuss whether this flag was ever intended to remain for as long as it has. Judging by the commit message, it does not seem like that was the case. I personally never use it as using a custom loader has always been my preference, but understand that there are those out there not interested in introducing the complexity necessary to achieve the same result using a custom module loader…

/cc @GeoffreyBooth

[ljharb]

The original intention was to gather feedback, and consider making it the default implementation.

However, there was never any clear rubric given for what would constitute "enough" feedback, so the likelihood the flagged behavior will ever become the default has rapidly dwindled. At this point, it's more likely to be removed entirely, I suspect.

[GeoffreyBooth]

understand that there are those out there not interested in introducing the complexity necessary to achieve the same result using a custom module loader…

/cc @GeoffreyBooth

On the contrary, nodejs/loaders#26 is about providing support to make it easy to reimplement this flag as a custom loader.

[lachrist]

Thank you all for your feedback! :) Maybe I put too much emphasis on the --experimental-specifier-resolution flag. My real motivation is to use the --experimental-loader flag to record loaded file. So I just want to point out that there is no easy solution to do so in presence of extension-less files.

// bin
import("./dep.mjs");

// dep.mjs
console.log("dep");

// loader.mjs
export const load = (url, context, loadDefaultAsync) => {
  console.log(`loading ${url}`);
  return loadDefaultAsync(url, context, loadDefaultAsync);
};

node --loader ./loader.mjs bin

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for ./bin

[lachrist]

On a related node, it might be useful to state in the doc that the use of --experimental-loader flag will cause the main module to always be loaded via the esm loader.

node/lib/internal/modules/run_main.js

Line 27 in 8c3637c

function shouldUseESMLoader(mainPath) {

.

[JakobJingleheimer]

If the issue is just for "bin" files, this is a duplicate of #41275 and #33226. For the previous issue, I did a quick workaround for this (testing with mocha actually), checking if the parent directory is "bin", and then returning defaultLoad(url, { ...context, format: "commonjs" }) (see my comment in 33226 for full code).

Are all "bin" files commonjs?

[lachrist]

Not necessarily bin files, any entry point (aka main files) which does not have an extension.

To answer your other question, bin files can be esm modules but it requires an extension: .mjs or .js (with configuration of the package json).

[JakobJingleheimer]

I would guess we would explicitly/automatically support "bin" files as that is a concept within Nodejs (but it has just been missed so far).

For other extension-less entry point files, I think that would likely be out of scope for Nodejs core.

There is discussion about exposing some of Nodejs's internal utilities related to this so users can do determine what they need as if in Nodejs core. One of those is a getPackageFormat, so you could easily do:

export async function load(url, context, defaultLoad) {
  const format = getPackageFormat(url);

  return defaultLoad(url, { ...context, format });

[lachrist]

@JakobJingleheimer, thanks for pointing me to these issues which are indeed related. And indeed this issue seem to present itself the most with bin files.

If I may, here is what I think is confusing with the current behavior of the loader. In general it is pretty easy to know which module loader will be invoked: import constructs will go to the esm loader and require invocations will directly go to the legacy cjs loader. However it less clear how the main module will be loaded because the presence of CLI flags can change that -- eg: --experimental-loader.

If I were to wild guess. I would say that the best solution would be to always use the esm loader for the main module. And make the esm loader load files without extension (or with .node extension) as commonjs files. Since we only increase the capabilities of the esm loader, this should not break backward compatibility?

[aduh95]

Duplicate of #38933.

[ljharb]

@lachrist a) the CJS loader isn’t legacy, and b) when type:module is used, it’s not so simple, and more cases than you might expect go to the ESM loader.

[bmeck]

If I were to wild guess. I would say that the best solution would be to always use the esm loader for the main module. And make the esm loader load files without extension (or with .node extension) as commonjs files. Since we only increase the capabilities of the esm loader, this should not break backward compatibility?

We actually did support extension-less files in the ESM loader at one time, this was ripped out due to forwards compatibility with WASM not backwards compatibility.

[lachrist]

Hi @ljharb I did not want to sound like the node team did something wrong. And I understand that there might be no easy fix for this. However the issue remains that this function shouldUseESMLoader directly affects the user, and is not mentioned in the module doc.

[GeoffreyBooth]

We actually did support extension-less files in the ESM loader at one time, this was ripped out due to forwards compatibility with WASM not backwards compatibility.

Yes, the decision was made to specifically not support ESM extensionless entry files because doing to would lock in their status as needing to be JavaScript, while we want to preserve the ability of Wasm extensionless entries in the future.

[aduh95]

shouldUseESMLoader directly affects the user, and is not mentioned in the module doc.

FWIW I'm adding this to the documentation in #41383.

[lachrist]

Thank you all for the follow up. It was very instructive. If someone stumble upon this issue and is wondering how to monitor node's module loading here is my solution.

Problem

At the moment, monitoring ECMAScript modules requires --experimental-loader flag. But this flag makes node always use the new esm loader to load the main file. Without this flag, node will sometimes use the old cjs loader. To be clear, the new esm loader is capable of loading commonjs files as well (along with native modules, web assembly modules, ...). So most of the time this change should not be noticeable. The biggest difference I encountered so far is that the cjs loader handles files without extension whereas the esm loader would throw an exception. This is intentional: the cjs loader only has to handle one kind of module (commonjs modules) whereas the esm loader has to handle different kinds of modules and the node team wants to reserve extension-less files for web assembly.

Solution

Add some logic to the loader to check for deviation:

let is_main = true;
export const load = (url, context, loadNext) => {
  if (is_main) {
    is_main = false;
    if (context.format === undefined) {
      context.format = "commonjs";
    }
  }
  return loadNext(url, context, loadNext);
};

Deprecated Solution

Monkey-patch the loading of the main file in a commonjs pre-loaded file. With something like this:

const Module = require("module");
const { realpathSync } = require("fs");
Module.runMain = (main = process.argv[1]) =>
  Module._load(realpathSync(main), null, true);

This simple solution assumes that the main file is a commonjs module, wich is not always the case. Also, it will not apply monkey-patches previously added to Module.runMain. To solve these two issues we need a slightly more elaborate and hacky patch such as:

const { copyFileSync, readFileSync, realpathSync, constants } = require("fs");
const { extname, dirname, basename, join } = require("path");
const Module = require("module");
const { runMain } = Module;

Module.runMain = function executeUserEntryPoint(main = process.argv[1]) {
  // TODO support --preserve-symlinks-main
  let path = realpathSync(main);
  const ext = extname(path);
  if (ext === "" || ext === ".node") {
    const new_path = join(dirname(path), `${basename(path, ext)}.cjs`);
    try {
      copyFileSync(path, new_path, constants.COPYFILE_EXCL);
    } catch (error) {
      if (error.code !== "ENOTSUP") {
        throw error;
      }
      if (readFileSync(path, "uft8") !== readFileSync(new_path, "utf8")) {
        throw new Error("Cannot solve extension-less main file issue");
      }
    }
    path = new_path;
  }
  return runMain(path);
};

[pencilcheck]

npx tsx solves everything, a.k.a. no config ts-node. No more stupid esm to commonjs conversion, and .ts EXTENSION NOT FOUND error anymore.