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: merge and simplify loader hooks #35524

Closed
wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Oct 6, 2020

Demo for reducing the number of hooks and allowing more delegation to the builtin hooks.

Downsides:

  • Eagerly loads source for JSON, even when it's not used.
  • If node thinks a module is CommonJS, it will currently not return the source from getSource. Since we now parse the CommonJS source for named exports, it would likely be fine if we'd return source from load anyhow but it's unclear how that would interact with userland hooks.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 6, 2020
@GeoffreyBooth
Copy link
Member

I’ve been looking into trying to implement loader chaining, where each user loader can modify the return value of the previous one(s), for all hooks. I think we need to merge the getFormat and getSource hooks to accomplish chaining, too, not just to solve #34144, so this PR would be a prerequisite for a chaining PR. I’ll try to find time soon to finish this up and get all the tests passing.

@jkrems jkrems mentioned this pull request Nov 30, 2020
9 tasks
JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this pull request Apr 18, 2021
nodejs#35524 did this (not sure whether it's necessary)
@GeoffreyBooth
Copy link
Member

Completed by #37468

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants