Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: Modify getFormat and getSource loader hooks #34144

Conversation

julian-londono
Copy link

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

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jun 30, 2020
@julian-londono julian-londono changed the title Modify getFormat and getSource loader hooks lib: Modify getFormat and getSource loader hooks Jun 30, 2020
@jkrems
Copy link
Contributor

jkrems commented Jun 30, 2020

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
Copy link
Member

devsnek commented Jun 30, 2020

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
Copy link
Contributor

jkrems commented Jun 30, 2020

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
Copy link
Contributor

jkrems commented Jun 30, 2020

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

@julian-londono
Copy link
Author

Looking forward to any further thoughts/insight on this!

@GeoffreyBooth GeoffreyBooth changed the title lib: Modify getFormat and getSource loader hooks esm: Modify getFormat and getSource loader hooks Jul 1, 2020
@jkrems jkrems added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Jul 1, 2020
@guybedford
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@guybedford
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

This sounds like a great concept to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants