doc: fix transpiler loader hooks documentation

[joyeecheung]

The loader hooks examples have been broken for a while:

1. The nextLoad() hook cannot be used on a .coffee file that ends
   up going to the default load step without an explict format,
   which would cause a ERR_UNKNOWN_FILE_EXTENSION. Mention
   adding a package.json with a type field to work around it
   in the example.
2. Pass the context parameter to the nextLoad() invocation and
   document that context.format is mandatory when module type
   is not explicitly inferrable from the module.
3. Correct the getPackageType() implementation which returns
   false instead of undefined in the absence of an explict format,
   which is not a valid type for format.

---

I still feel that moving the examples to a proper repository that have the examples actually tested in a workflow is the right way to go, which was why I left the TODO in L1348 instead of fixing the examples in any way some time ago. But until we do that, it's not good to have broken examples in the docs, so this just fixes it to be a working version without actually making it perfect in any way.

Also I feel that the current API is a bit awkward to use even for these examples:

1. The error ERR_UNKNOWN_FILE_EXTENSION isn't super helpful when any hooks are in place. This prevents using the next hooks for unknown extensions with format detection. It'll be more natural to either just treat unknown file extensions as .js text files by default, or make this behavior configurable in the context options.
2. There should be a helper for detecting the format based on user-provided texts + package.json types. Arguably this isn't the most optimial workflow that the customization hooks should have (we detect the type by compiling it as CJS first and then ESM, for the internal default load we can be smart enough to immediately go to subsequent steps once any of the compilation succeeds to avoid the extra compilation cost, but for the custom loader they would have to throw the module-detection compilation result away after getting the format in the load hook, but I guess at least having a helper is better than nothing)

These should probably be addressed in other PRs since they'll require changes to the API.

Refs: #57030

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[JakobJingleheimer]

1. The nextLoad() hook cannot be used on a .coffee file that ends up going to the default load step, which would cause a ERR_UNKNOWN_FILE_EXTENSION.

It should be fine as long as there is a resolve hook for it. I do this a bunch in https://www.github.com/nodejs-loaders/nodejs-loaders

2. The nextLoad() invocation needs the context parameter to be passed down.

That is not (or at least should not) be true: Node supplies context when it is not provided, and when it is provided, node merges in (with preference for provided props) missing properties (ex if you pass nextLoad(url, {}), the missing props are supplied by node). There are (or were) test cases that cover this. It is also working in nodejs-loaders/tsx (which I expect is a nearly identical case): https://github.com/nodejs-loaders/nodejs-loaders/blob/main/packages/tsx/tsx.loader.mjs#L71-L73 (And it has been working since before node added built-in typescript support).

You only need to provide context to nextLoad when you are changing/overriding what was passed into the current hook.

[joyeecheung]

It should be fine as long as there is a resolve hook for it.

I don't think it does? Adding a resolve hook that merely passes on the result of the nextResolve does not make the ERR_UNKNOWN_FILE_EXTENSION go away, and code-wise from defaultLoad and defaultLoadSync I can't see how a resolve hook would make a difference if the load hook does nextLoad('file:///url/to/something.coffee') that goes to the default loading.

[joyeecheung]

That is not (or at least should not) be true: Node supplies context when it is not provided, and when it is provided, node merges in

I see, then it's probably just the synchrnous hooks that is missing this - though this feels rather surprising to me, considering the return values are mandatory and does not accept merges, yet the context parameter is optional and can be merged...

[JakobJingleheimer]

That is not (or at least should not) be true: Node supplies context when it is not provided, and when it is provided, node merges in

I see, then it's probably just the synchrnous hooks that is missing this - though this feels rather surprising to me, considering the return values are mandatory and does not accept merges, yet the context parameter is optional and can be merged...

The reason for it was because I thought it was silly to require a hook to pass the thing just handed it by node back to node, the vast majority of the time totally unchanged. Or when it is changed, only 10% different (which would necessitate everyone doing the same thing, so I figured build that into node).

[joyeecheung]

The reason for it was because I thought it was silly to require a hook to pass the thing just handed it by node back to node, the vast majority of the time totally unchanged. Or when it is changed, only 10% different (which would necessitate everyone doing the same thing, so I figured build that into node).

I think the more surprising part is that this is not enforced consistently between return value and parameters - the return values might also be unchanged most of the time or only changed for a specific subset of modules. But yeah that's a bit off topic for this PR.

[joyeecheung]

It should be fine as long as there is a resolve hook for it.

I think I figured out why it seems to work in your examples - you likely always have an explicit known format (either in package.json, or extension, or the context.format parameter) when you invoke the load hook that evades the module format detection, and have not run into the case where there isn't a package.json with a type or otherwise explicit type (like what had been missing in the example and being called out in this PR). In this case format validation would run after module format detection and throw ERR_UNKNOWN_FILE_EXTENSION. This is exacerbated by the fact that context, and therefore context.format, is optional, so users are more likely give the default load step an undefined format that leads to ERR_UNKNOWN_FILE_EXTENSION if they are not careful in always giving Node.js a valid module type.

[joyeecheung]

Updated the PR to fix the incorrect package type implementation (it was returning false in the absence of explicit type) and warn about the format issue. @JakobJingleheimer can you take a look?

[JakobJingleheimer]

I think I figured out why it seems to work in your examples - you likely always have an explicit known format (either in package.json, or extension, or the context.format parameter) when you invoke the load hook that evades the module format detection

Yes, exactly: ERR_UNKNOWN_FILE_EXTENSION is thrown only via defaultGetFormat, which is called by resolve at the very end of the chain if format has not already been set to something, and by load when context.format is not set.

I wouldn't say "evades" (although I certainly see where you're coming from) as much as "gets out of the way": The design rationale here was intending to avoid "but it already knows!?" frustrations.

This is exacerbated by the fact that context, and therefore context.format, is optional, so users are more likely give the default load step an undefined format that leads

If I understand what you're saying, I think that is not correct: omitting context when calling nextLoad(url) does not cause context.format to get lost because node stitches it together (in nextHookFactory, called with context compiled from various places, but notably format via finalFormat from the end resolve chain). context.format getting "lost" would only happen when a user explicitly blocks it, like nextLoad(url, { format: '' }) (in which case, it's not so much "lost" as "shoved out of the boat").

[JakobJingleheimer]

@ovflowd in the incoming new API docs, will we have access to the awesome multi-file (tabbed) code samples like we do in Learn articles?

[joyeecheung]

If I understand what you're saying, I think that is not correct: omitting context when calling nextLoad(url) does not cause context.format to get lost

What I meant is, because context.format is not mandatory, it's easier to overlook the fact that format is mandatory when you are giving nextLoad() a file with unknown extension, not that it would get lost if it's already known.

[JakobJingleheimer]

What I meant is, because context.format is not mandatory, it's easier to overlook the fact that format is mandatory when you are giving nextLoad() a file with unknown extension, not that it would get lost if it's already known.

Ahhhh, gotcha. Perhaps we should call that out in the doc?

[joyeecheung]

Ahhhh, gotcha. Perhaps we should call that out in the doc?

It is already, from L1484

[JakobJingleheimer]

Ahhhh, gotcha. Perhaps we should call that out in the doc?

It is already, from L1484

I did not get that from L1484; I point I got from it is related specifically to the sample code (also important).

I would explicitly say something like:

When loading a format that is not recognised by Node.js, the corresponding resolve hook should return some value for format (a resolve hook is already necessary to avoid an earlier error). For instance, include format: 'coffee' in the return of resolve. Node with then get out of the way whilst loading occurs—but will still check that the final format from the load chain is a recognised value.

[joyeecheung]

I would explicitly say something like:

I don't think the resolve hook matters in this case. The problem comes from the nextLoad. Though the example reuse format for both nextLoad and the return value so it will have to be a recognized value. I can tweak the docs so that it hard-codes the format in the nextLoad hook.

[JakobJingleheimer]

I don't think the resolve hook matters in this case.

Hmm, perhaps not in this case. I think it's a good idea since you pointed out there is some arcana to this that even you did not get (maybe I'm the only person aware of this?). I'm happy to add it subsequently :)

[joyeecheung]

Updated the docs to point out context.format is mandatory if there's no explicit module type information.

[JakobJingleheimer]

❤️ 🙌

[nodejs-github-bot]

Landed in e892e79