-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fix: align behaviour to tsc rewriteRelativeImportExtensions
#17118
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
Fix: align behaviour to tsc rewriteRelativeImportExtensions
#17118
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58725 |
rewriteRelativeImportExtensions
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.
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 |
}); | ||
|
||
it.each([ | ||
// skip string wrapper object |
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.
The current Babel transform results fail these tests because we implicitly applied toString()
on the input.
471d21a
to
1f1541d
Compare
Ok, it turns the bug was intentional as tsc want to leave |
I also open a docs PR to clearly state that the |
I am a bit worried about the effects of this PR when it comes to 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? |
The TS team also intentionally does not support For the Since our |
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, |
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.
[^./]
Should we also include \
for windows paths?
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.
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.
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 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)) { |
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.
While we are changing this, could you wrap this in a !process.env.BABEL_8_BREAKING
?
@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. |
6d2dd23
to
080a4e9
Compare
080a4e9
to
ac2c6a7
Compare
Assuming this PR will be shipped in v7.27.0, I have updated the |
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 tscrewriteRelativeImportExtensions
option without the tsc JSX output option.