-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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(resolve): try .tsx extension for .js import from typescript module #7005
Conversation
packages/vite/src/node/utils.ts
Outdated
if (filename.endsWith('.js')) { | ||
const extensionLessFile = filename.slice(0, -3) | ||
return [`${extensionLessFile}.ts`, `${extensionLessFile}.tsx`] | ||
} |
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 believe the filename could potentially contain the ?
query parameter too (according to the regex), so checking endsWith
could be risky. We would need to handle that, e.g. filename is /foo/bar.js?bla
.
I'm also not sure if there's a case for .cjs
and .mjs
to map to .ctsx
and .mtsx
. But this PR would be in the next meeting to be discussed.
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.
Cool, will think about how to better handle cases with ?
after the extension. Also still need to add tests for resolve
. Let me know how the meeting goes. I'll be happy to update this PR based on any decisions made there.
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.
Added some tests resolve
:)
…or .mjs and .cjs files.
@leebeydoun We've discussed this a bit and think that this should be a fine addition. @aleclarson I'm interested in knowing your thoughts on this too as you had experience with these stuff. |
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.
LGTM. Would be great to have Alec's comment before merging too.
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 removed some failing tests, because they aren't calls to getPotentialTsSrcPaths
that actually ever occur in Vite, thanks to the isPossibleTsOutput
checks.
paths.push(name + type.replace('js', 'tsx') + query) | ||
} | ||
return 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.
This is really nice, thanks for improving the implementation I came up with.
Btw just curious, should |
I think the |
If (and only if) it doesn't work with native TypeScript / Node ESM, would it be better to disallow it? So that people don't write code that's not future proof |
Looks like TypeScript emits |
Description
Packages that use native ESM and TypeScript may import
.ts
files, which containimport
that end with a.js
extension. Vite currently tries to resolve these.js
extensions to.ts
,.ts.mjs
, etc. However, it doesn't try.tsx
.This PR fixes this, and ensures Vite also checks for a
.tsx
extension when the file is imported viaTypeScript
.Additional Context
closes #6866
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).