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.

[weswigham]

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

? "A tripleslash"?

[bmeck]

@weswigham https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

/// <reference strict="true"/>

That would be how I'd assume we would show intent in the docs if we expect it to be strict TS, as I don't think explaining how to add a JSON config for typescript is reasonable.

[weswigham]

We don't support anything like that right now. Triple slash directives only exist to specify file dependency ordering in legacy concatenate type code, mostly. (You can't set arbitrary compiler options)

as I don't think explaining how to add a JSON config for typescript is reasonable.

Rather than explaining, you could just... Add it. Like the lint config, to which it is very similar.

[bmeck]

@weswigham I don't understand. This is the documentation of the function, what do you mean "add it"

[weswigham]

Oh, I mean "add the config file to specify that you'd prefer stricter checks throughout the project".

[bmeck]

I'm remain not comfortable adding a TS config to the docs.

…

On Tue, Oct 1, 2019, 12:38 PM Wesley Wigham ***@***.***> wrote: Oh, I mean "add the config file to specify that you'd prefer stricter checks throughout the project". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29610?email_source=notifications&email_token=AABZJIZL4F3NVZMN3LCPAXDQMODHZA5CNFSM4IYFKIOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEACDC6A#issuecomment-537145720>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABZJI2XMIKWLGZKCUG6HNLQMODHZANCNFSM4IYFKIOA> .

[devsnek]

I think this is a bit of a red herring. We should not have jsdoc comments in code samples. The types are already specified exactly in the documentation.

[weswigham]

jsconfig.json

The tool may be named TypeScript, but ultimately there's no required exposure to TS as a language (unless you'd like to be pedantic and say the type documentation syntax is TS, but since it supports all the closure compiler syntax, too, I don't think that's really a problem), especially if you're just configuring IDE features when used this way.❤️ It's not much different from your eslint config file, really - it's a config file for a tool that runs checks over js files, the primary difference being this one does semantic checks and improves IDE features in some popular editors - especially if you're only using it for the editor support, and not actually running the tool for errors in CI. I somehow doubt we'd be having quite the same discussion if the js checker and the ts checker were different libraries, but they're not (since they're almost identical), so here we are.

Plus, I, at least, think a single config file is a bit nicer than inline pragmas for configuration - there's a reason every file doesn't list the lint rules it enables at the top; it's a bit messy, and usually repetitive. (Which, I guess, is why you said you wouldn't want to add one, if one worked, yeah?)

In any case, I wouldn't write inaccurate documentation, checked or no - the docs should include the undefined-ness of a param in some way. 🤷‍♂️

[weswigham]

The types are already specified exactly in the documentation.

Aren't we talking about the code comments in the implementation? Oh, I had no idea we were talking about the doc sample, totally misread that. 🤦 (I thought we were talking about docstrings on the actual internals, which I'd love to see more of, cause they're pretty rare right now.)

I think stating the types in the sample is convenient (since it condenses what you need to read to understand the functionality), it's just unfortunately hard to keep it in sync with the API's types. Oh, and it being inaccurate messes with editor intellisense when people copy & paste it into their codebase, so if it's there, it should be precise~

[DerekNonGeneric]

Opinions about --experimental-loader vs --loader would be appreciated. The flag just landed in v12.11.1 (#29796). It seems like the docs are recommending it although both flags seem viable.

[bmeck]

@DerekNonGeneric I'd use the --experimental-loader

[DerekNonGeneric]

@devsnek, should I modify my PR to remove the type annotations?

[devsnek]

@DerekNonGeneric i'm not planning to block it or anything, but i don't really see the point of including them.

[DerekNonGeneric]

The ES module dummy loader has since been removed from this document and replaced with several wonderful examples. 🎉