X Tutup
The Wayback Machine - https://web.archive.org/web/20220504005219/https://github.com/nodejs/node/pull/35524
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
Closed

Conversation

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

@nodejs-github-bot nodejs-github-bot commented Oct 6, 2020

Review requested:

@nodejs-github-bot nodejs-github-bot added the esm label Oct 6, 2020
@GeoffreyBooth
Copy link

@GeoffreyBooth GeoffreyBooth commented Oct 8, 2020

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 issue Apr 18, 2021
nodejs#35524 did this (not sure whether it's necessary)
@GeoffreyBooth
Copy link

@GeoffreyBooth GeoffreyBooth commented Sep 12, 2021

Completed by #37468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants
X Tutup