Skip to content

ES module source transforms#29924

Closed
coreyfarrell wants to merge 1 commit into
nodejs:masterfrom
coreyfarrell:esm-transform-v2
Closed

ES module source transforms#29924
coreyfarrell wants to merge 1 commit into
nodejs:masterfrom
coreyfarrell:esm-transform-v2

Conversation

@coreyfarrell

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

ljharb commented Oct 10, 2019

Copy link
Copy Markdown
Member

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

@devsnek

devsnek commented Oct 10, 2019

Copy link
Copy Markdown
Member

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

@coreyfarrell

Copy link
Copy Markdown
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?

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

devsnek commented Oct 10, 2019

Copy link
Copy Markdown
Member

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

Trott commented Oct 13, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

Closing since #30986 is merged.

@GeoffreyBooth

Copy link
Copy Markdown
Member

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

@coreyfarrell

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