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

Support import() in rewriteImportExtensions #16794

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

liuxingbaoyu
Copy link
Member

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

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 2, 2024

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57831

}
},
ImportExpression(path) {
maybeReplace(path.node.source);
Copy link
Member

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));
Copy link
Member

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.

Copy link
Contributor

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.

Comment on lines 4 to 6
return /[\\/]/.test(source)
? source.replace(/(\.[mc]?)ts$/, "$1js").replace(/\.tsx$/, ".js")
: source;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 3, 2024

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)
Copy link
Contributor

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 .

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 3, 2024

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.

@nicolo-ribaudo nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Sep 3, 2024
@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Sep 6, 2024
@nicolo-ribaudo nicolo-ribaudo added this to the v7.26.0 milestone Sep 6, 2024
@nicolo-ribaudo nicolo-ribaudo merged commit bfa56c4 into babel:main Oct 23, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: @babel/preset-typescript rewriteImportExtensions does not work for dynamic imports
5 participants