feat(es/module): support moduleRoot for AMD module IDs - #11964
feat(es/module): support moduleRoot for AMD module IDs#11964Kaustubh Singh (Kaus1ubh) wants to merge 3 commits into
Conversation
|
Merging this PR will degrade performance by 2.95%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d915ee1010
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Thanks for working on this. Before moving forward, I think we should step back and revalidate whether this feature should be added to core at all. A feature request does not necessarily mean that we should implement it, especially when it introduces SWC-specific, non-standard configuration surface area and the underlying issue has been stale for quite a while. Could you first confirm a few things?
If there is no current demand or no clear precedent in Babel or TypeScript, I would lean toward not adding this to core for now. |
|
thanks for the feedback magic-akari , On the use case, from what I can see in #4014 , the original requester was still interested as recently as 2023 and the issue has been open for 4+ years without anyone proposing a workaround, so it does seem like a real gap people have been running into. Yes both Babel and TypeScript have something like this, @babel/plugin-transform-modules-amd has a moduleRoot option that works the same way, and TypeScript produces similar behavior through --rootDir when compiling to AMD. So the intent was to mirror existing behavior rather than introduce something new. I'm still pretty new to the codebase so I might be missing important context around why this was left out originally or what the longer-term direction for AMD support looks like. Happy to hear your thoughts on whether this fits where the project is headed. |
|
Thanks for the clarification. When we initially compared the implementation with TypeScript, we overlooked how After taking another look, TypeScript’s I also noticed that TypeScript 7 has removed AMD output support entirely, which is somewhat disappointing. |
|
magic-akari the outFile vs outDir difference in TypeScript makes sense. I think I was leaning on that comparison a bit too loosely, and with TypeScript 7 dropping AMD altogether it's probably not the best reference point anyway. Using Babel as the reference sounds right to me. From what I can tell the current implementation is already pretty close to how @babel/plugin-transform-modules-amd handles moduleRoot, computing the module ID as the relative path from the root to the current file. But I'd be happy to do a more careful side-by-side comparison and tweak anything that doesn't line up. what should we do here? |
|
To clarify, the points I raised are no longer blockers for this PR. It can now proceed through the usual review and merge process. Thank you for your patience. |
Closes #4014
Description
This PR adds support for the moduleRoot configuration property in the AMD module transformer.
When you specify a moduleRoot, SWC will now compute the module ID based on the relative path between that root directory and the file currently being compiled. The goal here is to make SWC perfectly mimic TypeScript's AMD path generation behavior!
I've also made sure we handle edge cases correctly:
BREAKING CHANGE:
None.
Related issue:
#4014