I talked with @GeoffreyBooth and am fine with landing this as long as we make the potential for change much more visible as we have discussed other hooks and I synced up with him about how these hooks are different from the design docs hooks we have previously discussed. We should sync up about the design of these hooks being explicitly written out, but this is fine as long as we go with an iterative approach and do not commit to these as final hooks.

Do you think you could set up a benchmark? It would be good to keep a handle on how our loader performance is, and this seems like it might add a lot of overhead (not that I'm saying we shouldn't land it, but rather we should just be aware).

Do you think you could set up a benchmark?

That would be great, but I’ve never done benchmarks in the Node codebase. Is there something you can point me to to show me how? Can that be its own PR?

Also more broadly how would that work? Like I would assume that the benchmarked case is “no custom loaders,” so is what we’re benchmarking just how much slower that case becomes as more hooks are added? Or is your thinking to benchmark “no loaders” vs “with custom loader A” or B or C etc.?

@GeoffreyBooth benchmarks are here: https://github.com/nodejs/node/tree/master/test/benchmark. From what I can tell they run a specified test a bunch of times. The case I have in mind at the moment is "resolve big tree without hooks", specifically I'm eyeing the added await and two branches in each translator path, which is why I thought it would be good to add the benchmark now. Adding it later also works too.

Adding it later also works too.

Thanks. I’m not sure when I’ll have time for that, so if you don’t mind I’d prefer to do that as a follow-up PR (or anyone else is welcome to).

As discussed further with @GeoffreyBooth I'd be happy to have a getFormat hook that can be called after resolve, and before getSource.

As discussed further with @GeoffreyBooth I'd be happy to have a getFormat hook that can be called after resolve, and before getSource.

So the current flow works like this:

1. The ESM loader receives a specifier, e.g. 'some-pkg/some-export'
2. resolve converts that specifier into a file URL, e.g. 'file:///.../node_modules/some-pkg/lib/some-export.js'
3. resolve additionally determines the format by looking at the file extension of the URL in 2 and possibly referencing package type. format in this case is the name of one of the translators: module, commonjs, builtins, wasm, json, dynamic
4. Based on the format returned, one of the translators is chosen and passed the URL
5. Inside the translator, getSource retrieves file contents for the given URL (and transformSource possibly mutates the contents)

So a getFormat hook would split out that logic from inside resolve (i.e. make step 3 above its own hook). I think that would be all we do for the first version of the hook.

Later on we probably want the “determine format” logic to be possibly set or overridden by the source returned in getSource, for example if getSource makes an HTTP call and we want the format set based on the headers in the response. I would still want getFormat to be its own hook, but we’d have to reorder the steps of the flow or make it optional when the format is determined. That’s a bigger refactor, possibly a redesign of this whole flow, that I think would be a separate effort.

I would still prefer to only have a getSource hook and not also add a transform hook, since the ability to transform is enabled by the hook composition model, and I don't like the idea of endorsing the concept of transform hooks in the API explicitly, as opposed to simply enabling them.

Combining them would cause problems once adding multiple hooks is supported. getSource callbacks are not necessarily expected to call the defaultGetSource but every registered transform hook should be run on every source. Sorry that I'm not understanding, can you tell me the reason for opposing direct support for transform hooks?

Combining them would cause problems once adding multiple hooks is supported. getSource callbacks are not necessarily expected to call the defaultGetSource but every registered transform hook should be run on every source.

Yes, with the getSource model, we can still have both classes of hooks, but they follow different rules - source hooks ignore defaultGetSource, while transform hooks implemented via getSource always chain with defaultGetSource (which under loader composition would be the parent loader return value).

Sorry that I'm not understanding, can you tell me the reason for opposing direct support for transform hooks?

Great question - the primary reason is a human / branding one. Including a transform hook explicitly in the Node.js documentation acts as an advertisement to users to write their own transform loaders, and that leads to a bad time unless loader authors are highly skilled at implementing caching, from my experience with SystemJS.

I've extracted a getFormat hook based upon conversations. There is some dancing going on so that old resolve hooks are properly maintained to minimize migration breakage. Ideally we can remove some of the odd ordering of operations in the future when we fully remove format from resolve.

Sorry that I'm not understanding, can you tell me the reason for opposing direct support for transform hooks?

Great question - the primary reason is a human / branding one. Including a transform hook explicitly in the Node.js documentation acts as an advertisement to users to write their own transform loaders, and that leads to a bad time unless loader authors are highly skilled at implementing caching, from my experience with SystemJS.

@guybedford I'm worried that if transformSource is omitted transform hooks will get blocked/ignored by fetch hooks. Specifically I'm concerned that this will introduce a situation where a fetch hook added in the incorrect order will cause the nyc transform (instrument) function to simply never run. No error will be provided to the user, they just won't get coverage.

I appreciate your not wanting to encourage unnecessary transform hooks but can this be solved by documentation warnings? In addition I have not seen firm plans for an API to add loader hooks. At one point I was hoping for an API to enable require('./install-a-loader-hook.js') but your comment about SystemJS has given me reason to reconsider this. If --loader will be the only way to add a hook this is essentially a permanently flagged feature.

When running yarn install, we generate a single .pnp.js file that contains the CJS loader and inject it in the environment through --require. For the loader API to be viable for us we'd need this .pnp.js file to keep working as it does while also being a valid loader. I'm not sure that's the case: the .pnp.js file will be CJS (and need to, for backward compat), whereas the loader has to be ESM from what I gather.

Why are loaders required to be ESM? As it been considered to also support CJS-exported loaders?

maybe look into my implamentation https://github.com/direktspeed/esm-loader it works without hooks and is highly configurable via parsers

I pushed some commits last night but this is still WIP. A few TODOs:

• getFormat currently needs to be synchronous, but we should allow a getFormat hook to be async.

• Since getFormat always returns an object, e.g. {format: 'module'}, the other new hooks should also always return objects, e.g. {source: '...'}.

• We should probably remove loader from all of the hook signatures.

• How do people feel about the unified signatures across all hooks? They’re now all hook(options, defaultHook) e.g. resolve({ specifier, parentURL }, defaultResolve) and so on. And all would return objects.

I updated the docs to move the loader examples into their own section, where I think they make much more sense rather than haphazardly spread across multiple hooks’ sections. I think that’s everything, @guybedford please let me know if there’s anything else.

CI: https://ci.nodejs.org/job/node-test-pull-request/28231/

Once CI completes it looks like we will be good to land this. Further final reviews welcome.

Landed in 2551a21.