-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Support import()
in rewriteImportExtensions
#16794
Conversation
liuxingbaoyu
commented
Sep 2, 2024
Q | A |
---|---|
Fixed Issues? | Fixes #16750 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57831 |
} | ||
}, | ||
ImportExpression(path) { | ||
maybeReplace(path.node.source); |
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.
I saw that the TS team was also considering using a runtime helper to replace the extension in cases where the specifier is not statically analyzable. We should consider doing something like that.
@@ -0,0 +1,2 @@ | |||
import("./a.js"); | |||
import(babelHelpers.replaceTsImportExt(a)); |
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.
a
might not be a string -- in that case we need to do String(a)
first.
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.
I think a + ""
should align to the ToString
AO better than String()
, since String(symbol)
will return its description while ToString(symbol)
is a type error.
return /[\\/]/.test(source) | ||
? source.replace(/(\.[mc]?)ts$/, "$1js").replace(/\.tsx$/, ".js") | ||
: source; |
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.
This could probably be shorter:
return String(source).replace(/([\\/].*\.[mc]?)tsx?$/, "$1js");
and it becomes short enough that we could even just inline it in the code without a helper, so we don't have the problem of it not being available.
/* @minVersion 7.25.7 */ | ||
|
||
export default function replaceTsImportExt(source: string) { | ||
return /[\\/]/.test(source) |
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.
Here, it is not sufficient to detect the bare import; consider @foo/bar.ts
.
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.
That might need to be rewritten, if for example you are remapping
<script type="importmap">
{
"imports": {
"@components/": "./ui/components"
}
}
</script>
We added the exception for "it needs to have a slash" because:
- there are known npm packages with no scope that end in
.ts
- if you use import maps and have no slash, it means you are explicitly writing the import specifier in your import map
So far nobody ever reported a problem with the @foo/bar.ts
handling, but if there is a package out there needing it the only way forward is to provide a function option in the config once somebody needs it.