-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
ES module source transforms #29924
ES module source transforms #29924
Conversation
cc @nodejs/modules-active-members; not sure this is something we've previously discussed |
would you mind terribly removing the chaining commit? that's a larger thing we have to deal with for all loaders. |
0e61020
to
d30faa9
Compare
@devsnek done. note even in the first commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing something like this. It's been a gap for a while.
What about JSON / Wasm and binary files? Should this instead be called moduleTransform then?
A fetch hook that can return binary would apply to all formats, so you can get Wasm, JSON and CJS through patching the CJS loader. Yes, applying to Node binaries is harder, but that's the main exception then.
Personally I would prefer to see that sort of approach to a transform.
@guybedford renaming to An earlier attempt at this feature allowed |
I think this needs more abstract design work before we start throwing more hooks in. |
I've labeled this "work in progress" but comment or remove that label if it's not appropriate. I'd prefer a bit more detail in the commit message if possible. |
Closing since #30986 is merged. |
Oh wow I didn't even know that this PR was here. |
@GeoffreyBooth I dropped the ball on this, other priorities unfortunately took over. I really appreciate the effort you took to get the transform hook implemented and merged! |
Checklist
make -j4 test
passes