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

ES module source transforms #29924

Closed
wants to merge 1 commit into from

Conversation

coreyfarrell
Copy link
Member

Checklist
  • make -j4 test passes
  • tests are included
  • documentation is changed or added - needs improvement, I'd appreciate advice on this
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 10, 2019
@ljharb
Copy link
Member

ljharb commented Oct 10, 2019

cc @nodejs/modules-active-members; not sure this is something we've previously discussed

@devsnek
Copy link
Member

devsnek commented Oct 10, 2019

would you mind terribly removing the chaining commit? that's a larger thing we have to deal with for all loaders.

@coreyfarrell
Copy link
Member Author

@devsnek done. note even in the first commit Loader#_transformSource holds an array rather than an undefined or single function, so once it's possible to have multiple --loader arguments the transformSource will automatically support chaining. Do you want this changed as well?

Copy link
Contributor

@guybedford guybedford left a 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.

@coreyfarrell
Copy link
Member Author

@guybedford renaming to moduleTransform or maybe transformModuleSource could make sense as this implementation supports ES modules only.

An earlier attempt at this feature allowed resolve hooks to provide a fetchSource property, but this cannot effectively support chained transforms. Keeping fetch and transform separate makes it possible to support multiple --loader options each providing a transform to be run in sequence. I think a fetch hook is a good idea but a separate feature.

@devsnek
Copy link
Member

devsnek commented Oct 10, 2019

I think this needs more abstract design work before we start throwing more hooks in.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Oct 13, 2019
@Trott
Copy link
Member

Trott commented Oct 13, 2019

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.

@coreyfarrell
Copy link
Member Author

Closing since #30986 is merged.

@GeoffreyBooth
Copy link
Member

Oh wow I didn't even know that this PR was here.

@coreyfarrell
Copy link
Member Author

@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!

@coreyfarrell coreyfarrell deleted the esm-transform-v2 branch January 7, 2020 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants