esm: Modify getFormat and getSource loader hooks

[julian-londono]

This PR aims to amend the current behavior of the getFormat and getSource experimental loader hooks by allowing getSource to optionally override the format of the source that had been defined by getFormat.

This is an active WIP ; I am working under the guidance of contributor @jkrems

Checklist

• make -j4 test (UNIX) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[jkrems]

This came up when trying to implement hooks that would get sources from a location with its own meta data (e.g. HTTPS). The current model requires that the format is determined before trying to load the source which isn't really possible there. Allowing getSource to provide the format removes the need for storing state in the hooks.

[devsnek]

I agree with the motivation but I'm not sure about this specific solution, specifically with the disagreement among hooks as that could be difficult to debug.

[jkrems]

specifically with the disagreement among hooks as that could be difficult to debug.

One thing @GeoffreyBooth brought up: If the hooks get reshuffled, it may be possible to only get format from one source.

1. Call getSource. It may return {source} or {source, format}.
2. Only if getSource didn't return a format, call getFormat.

The downside is that we'd have to decide what the default getSource hook does. If it returns a format, it would potentially help other getSource implementations but it would make getFormat into a hook that doesn't run by default (?).

I think my current vote would be to reorder (call getSource first) and not return a format from defaultGetSource. A close second would be merging getSource and getFormat officially but that may be exploding the scope of this PR.

[jkrems]

/cc @nodejs/modules-active-members (since the discussion has started anyhow :))

[julian-londono]

Looking forward to any further thoughts/insight on this!

[guybedford]

@julian-londono just post a comment when you are ready for further review here.

Another question I had about this - would it make sense for the transformSource hook to also have the ability to redefine the format?

[julian-londono]

Another question I had about this - would it make sense for the transformSource hook to also have the ability to redefine the format?

Good question, I could see this having valid use cases, but it was not under the original scope of this change - I can explore this more and get back to you @guybedford

[julian-londono]

I've created an doc summarizing my exploration into this issue and potential solutions (RFC):
https://docs.google.com/document/d/1kT3HvrPntjG94NeEbJq1O7XykWrAWYuq6U04whsgrcI/edit?usp=sharing

@guybedford : We think that allowing transformSource to also optionally change the format is out of scope for this PR. Furthermore, it would go against the idea of what transformSource is - it would mean that it does more than just "transforming" the source if it also changes the format

[julian-londono]

Looking forward to some thoughts about my exploration into the issue - after this I could move forward with adding potential documentation updates 👍

[guybedford]

It still feels to me a little inconsistent to have transformSource not fit the model. For example, how would a loader be written to support TypeScript if it were to compose with an existing loader that returned the source from getSource? It would need to override both hooks... most users wouldn't hit that until the composition case actually happens making this somewhat of a composition footgun.

Trying to think of alternatives here... would it be an option to reorder getFormat to be called after getSource so that the internal cache can be used without needing to trigger the fetch separately?

[julian-londono]

Seems like a good alternative is to remove the getFormat hook as a whole (idea developed by @jkrems). In that case the hooks would roughly look like the following:

• resolve(specifier, parent) → → {url}
• getSource(url) → {source, format}
• transformSource(url, format, source) → {source, format}

In this case, transformSource "could be sugar for a getSource hook that always calls defaultGetSource" ( - @jkrems). I agree with this and can start on a new PR in the coming days.

[guybedford]

This sounds like a great concept to me!