doc: ES module dummy loader resolve hook bug on Windows

[DerekNonGeneric]

• Version: 12.10.0
• Platform: Windows Server 2019
• Subsystem: url

An error occurs after creating the custom-loader.mjs containing the dummy loader code as directed by the documentation, an x.js file, and running the following command in PowerShell.

node --experimental-modules --loader ./custom-loader.mjs x.js

By the way, this command diverges from the one provided in the documentation to be used on Windows as NODE_OPTIONS='--experimental-modules --loader ./custom-loader.mjs' node x.js fails in PowerShell.

(node:1364) ExperimentalWarning: The ESM module loader is experimental.
(node:1364) ExperimentalWarning: --loader is an experimental feature. This feature could change at any time
internal/modules/cjs/loader.js:992
      internalBinding('errors').triggerUncaughtException(
                                ^

TypeError [ERR_INVALID_URL]: Invalid URL: /C:/Users/Administrator/source/repos/esm/x.js
    at onParseError (internal/url.js:243:9)
    at new URL (internal/url.js:319:5)
    at resolve (file:///C:/Users/Administrator/source/repos/esm/custom-loader.mjs:23:20)
    at Loader.resolve (internal/modules/esm/loader.js:73:33)
    at Loader.getModuleJob (internal/modules/esm/loader.js:152:40)
    at Loader.import (internal/modules/esm/loader.js:136:28)
    at internal/modules/cjs/loader.js:989:27 {
  input: '/C:/Users/Administrator/source/repos/esm/x.js'
}

The x.js file certainly exists. It turns out that there is a Windows-specific error where a leading forward slash causes this example to break. I was able to resolve this by adding:

  specifier = cleanPath(specifier);
  specifier = url.pathToFileURL(specifier).href;

prior to (from the dummy loader code):

 const resolved = new URL(specifier, parentModuleURL);

and including the following function:

/**
 * Path sanitizer that removes a leading slash if followed by Windows drive sepcifier.
 * @param {string} specifier URL path to a file
 * @returns {string} Cleaned specifier URL path to a file
 */
function cleanPath(specifier) {
  const specifierDir = path.parse(specifier).dir;

  if (
    specifierDir.length >= 3 &&
    specifierDir.charAt(0) == '/' &&
    specifierDir.charAt(1).toUpperCase() !=
      specifierDir.charAt(1).toLowerCase() && // Check if alphabetic
    specifierDir.charAt(2) == ':'
  ) {
    specifier = specifier.substring(1);
  }

  return specifier;
}

P.S. This also needs import url from 'url'; added to the top of custom-loader.mjs.

[joaocgreis]

cc @nodejs/modules-active-members

[bmeck]

This looks like a bug with resolving argv[1] during bootstrapping.

Edit: https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L1005 looks suspect

[bmeck]

@DerekNonGeneric do you have your code for custom-loader.mjs, Are you using url.fileURLToPath() ?

[DerekNonGeneric]

Thanks for looking into this @bmeck. I am indeed using url.fileURLToPath(). My code for custom-loader.mjs has changed significantly since I opened this issue, but I've tried to bring it back down to a comprehensible reduction to help out anyone looking into this. Apologies in advance for changing the formatting (different style guide).

https://gist.github.com/DerekNonGeneric/bdf314493c3262c93ceab6fda1c7695b

[guybedford]

Changing https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L1005 to use .href over .pathname seems like it would fix the issue here to me. Thanks for finding this bug @DerekNonGeneric.

[DerekNonGeneric]

A few documentation-related errors I see:

• The error stated is incorrect.

  imports must begin with '/', './', or '../'; '${specifier}' does not
  

  According to the Node.js documentation specifiers may not begin with /.

• URL does not need to be imported, since it is already in scope.

• The JSDoc comment at the top should be of type {string=} as it is optional parameter, yet it reads:

   * @param {string} parentModuleURL
  

• The command given to execute this code is not cross-platform. Environment variables cannot be passed to commands like this in PowerShell.

  NODE_OPTIONS='--experimental-modules --loader ./custom-loader.mjs' node x.js

  it should instead read:

  node --experimental-modules --loader ./custom-loader.mjs x.js

  or possibly

  node --experimental-modules --experimental-loader ./custom-loader.mjs x.js

/to @bmeck, @guybedford Do y'all mind if I make a PR to fix these?

[bmeck]

I'd avoid the JSDoc change, there is some disagreement and I lean towards TS syntax which does not enforce non-undefined value:

/**
 * @param {string} x
 */
function foo(x) {
  return x;
}
foo(undefined);

[jkrems]

TS syntax which does not enforce non-undefined value

Not sure how relevant that is here but with strict: true (or just strict null checks), TS does mark your sample as an error. I think it's reasonable to be explicit about optional and/or nullable parameters in the API.

[weswigham]

FYI, that does enforce non-undefined if strict (or at least strictNullChecks) is on. (And it should be on.) It should be string= if it's optional, which the TS checker'll also recognize.

[bmeck]

I'm not really cozy on relying on non-default configuration to determine that.

[jkrems]

It doesn't break in the default configuration to use string=. And, as Wes said, strictNullCheck isn't an obscure setting but pretty close to an accepted best practice. Your sample relies on the fact that TS actively ignores (some) null errors in the default configuration to make transitions easier (I assume).

[bmeck]

even if we have a recommendation, we should not do something if it not the default. E.g. don't share strict mode code if the default is non-strict unless you express that it should be strict. We could argue about this a bit but overall it isn't as important, I'd be fine with string | undefined since it always populated however. I'd not be ok with treating it as optional if it always does have a value passed in.

[weswigham]

string | undefined works, yeah. If it's always supposed to be provided, i'd recommend that - and the closure compiler typesystem would, too.

don't share strict mode code if the default is non-strict unless you express that it should be strict.

You can drop a jsconfig.json (or tsconfig.json) somewhere at the root of the js source to set options for vscode and other TS-based JS's IDEs. The default for a new one generated by tsc is strict: true, but that'll pepper errors all over the codebase, likely, since so much internal stuff is missing docs. Then again, those errors won't appear anywhere unless you set the checkJs option or drop a //@ts-check in a file, so... Eh.

Point is, the intention can be conveyed, if you'd like.

[bmeck]

Point is, the intention can be conveyed, if you'd like.

I'm not eager to add a tripleslash to our docs for Node.js itself.