Skip to content

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Feb 7, 2025

Q                       A
Fixed Issues? Fixes #17114
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link babel/website#3052
Any Dependency Changes?
License MIT

In this PR we introduced a new helper tsRewriteRelativeImportExtensions that is almost identical (the regex is slightly improved with the help of eslint-regex) to the TS helper. The new behaviour in this PR should mirror the tsc rewriteRelativeImportExtensions option without the tsc JSX output option.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: typescript labels Feb 7, 2025
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 7, 2025

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

@JLHwung JLHwung changed the title Fix ts rewrite relative import extension Fix: align behaviour to tsc rewriteRelativeImportExtensions Feb 7, 2025
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would just tweaking our original regexp to this have the same effect?

.replace(/^(\.\.?[\\/].*\.[mc]?)tsx?$/, "$1js")

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 9, 2025

Would just tweaking our original regexp to this have the same effect?

.replace(/^(\.\.?[\\/].*\.[mc]?)tsx?$/, "$1js")

Good question. I think this one still deviates from the TS one: 1) It does not handle .d.ts and 2) It does not support case-insensitivie variants such as .mTs. Though the latter is trivial to fix, the former still requires longer regex. I added a unit test for the new helper and it turns out we have found a new bug in this PR, I will fix that later.

});

it.each([
// skip string wrapper object
Copy link
Contributor Author

@JLHwung JLHwung Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current Babel transform results fail these tests because we implicitly applied toString() on the input.

@JLHwung JLHwung force-pushed the fix-ts-rewrite-relative-import-extension branch 2 times, most recently from 471d21a to 1f1541d Compare February 10, 2025 16:26
@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 10, 2025

Ok, it turns the bug was intentional as tsc want to leave .d.css.ts intact. I have reverted to the original implementation and added more test cases. This PR is ready to review.

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 10, 2025

I also open a docs PR to clearly state that the rewriteImportExtensions option mirrors the tsc rewriteRelativeImportExtensions option: babel/website#3052

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 10, 2025

I am a bit worried about the effects of this PR when it comes to from "@some/package/foo.ts: the current behavior is intentional (#15913 (comment)) and can be useful for example when working with monorepos and importing specific files from other packages.

Do you think it'd be possible for now to only change the behavior for absolute specifiers, and keep the complete alignment for Babel 8? Or is it too difficult to detect?

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 10, 2025

The TS team also intentionally does not support @foo/bar.ts: microsoft/TypeScript#59767 (comment).

For the @some/package/foo.ts usage, I think it'd better resort to the extension-less sub-exports like @some/package/foo. I just checked our monorepo and fortunately, we don't have such usage. I suggest we ship it in the next minor and gather some feedback. If there is strong pushback, we can defer the scoped package part to Babel 8.

Since our rewriteImportExtensions option (Babel 7.23 in Sep 2023) preceded tsc's rewriteRelativeImportExtensions option (TS 5.7 in Nov 2024), it can be understood that they are not aligned at this moment. The sooner we close this gap, the less friction the ecosystem will feel.

@nicolo-ribaudo
Copy link
Member

Ok, let's do it.

if (/^\.\.?\//.test(source.value)) {
// @see packages/babel-helpers/src/helpers/tsRewriteRelativeImportExtensions.ts
source.value = source.value.replace(
/\.(tsx)$|((?:\.d)?)((?:\.[^./]+)?)\.([cm]?)ts$/i,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[^./]

Should we also include \ for windows paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. TS currently does not handle \: https://www.typescriptlang.org/play/?rewriteRelativeImportExtensions=true#code/JYWwDg9gTgLgBAIgHRIDoDMISTAzggKFElkRQHpNs8Eg

I think we should handle the windows path separator as well. I will open a new issue to the TS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and can confirm the Node.js ESM does not support relative imports with Windows path separator. I changed my mind and reverted the last commit.

}
},
CallExpression(path) {
CallExpression(path, state) {
if (t.isImport(path.node.callee)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are changing this, could you wrap this in a !process.env.BABEL_8_BREAKING?

@nicolo-ribaudo
Copy link
Member

@JLHwung This can go in a patch release, right?

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 12, 2025

@JLHwung This can go in a patch release, right?

I think it's time for a new minor given that 7.26 was released 3 months ago. If so maybe this PR can be shipped with 7.27.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.27.0 milestone Feb 13, 2025
@JLHwung JLHwung force-pushed the fix-ts-rewrite-relative-import-extension branch from 6d2dd23 to 080a4e9 Compare February 14, 2025 18:37
@JLHwung JLHwung force-pushed the fix-ts-rewrite-relative-import-extension branch from 080a4e9 to ac2c6a7 Compare February 14, 2025 18:39
@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 14, 2025

Assuming this PR will be shipped in v7.27.0, I have updated the minVersion and cleared the todo item.

@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 Mar 20, 2025
@nicolo-ribaudo nicolo-ribaudo merged commit ca4865a into babel:main Mar 21, 2025
55 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the fix-ts-rewrite-relative-import-extension branch March 21, 2025 11:27
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 21, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 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.

@babel/preset-typescript: have a flag disable rewriteImportExtensions for dynamic imports
4 participants