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

Jump to bottom

fix(resolve): try .tsx extension for .js import from typescript module #7005

Merged
merged 7 commits into from Mar 2, 2022
Merged

fix(resolve): try .tsx extension for .js import from typescript module #7005

merged 7 commits into from Mar 2, 2022

Conversation

leebeydoun
Copy link
Contributor

@leebeydoun leebeydoun commented Feb 20, 2022 •
edited

Description

Packages that use native ESM and TypeScript may import .ts files, which contain import 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 via TypeScript.

Additional Context

closes #6866

What is the purpose of this pull request?

  • Bug fix

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@leebeydoun leebeydoun changed the title fix: ensure typescript resolution considers both .js and .jsx extensions fix #6866 fix: ensure typescript resolution considers both .js and .jsx extensions fix #6866[,#6866] Feb 20, 2022
@leebeydoun leebeydoun changed the title fix: ensure typescript resolution considers both .js and .jsx extensions fix #6866[,#6866] fix: ensure typescript resolution considers both .js and .jsx extensions fix #6866 Feb 20, 2022
@leebeydoun leebeydoun changed the title fix: ensure typescript resolution considers both .js and .jsx extensions fix #6866 fix: ensure typescript resolution considers both .js and .jsx extensions (fix #6866) Feb 20, 2022
@Niputi Niputi changed the title fix: ensure typescript resolution considers both .js and .jsx extensions (fix #6866) fix: ensure typescript resolution considers both .js and .jsx extensions Feb 20, 2022
bluwy
bluwy reviewed Feb 22, 2022
View reviewed changes
packages/vite/src/node/utils.ts Outdated
Comment on lines 195 to 198
if (filename.endsWith('.js')) {
const extensionLessFile = filename.slice(0, -3)
return [`${extensionLessFile}.ts`, `${extensionLessFile}.tsx`]
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests resolve :)

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 25, 2022
@bluwy bluwy changed the title fix: ensure typescript resolution considers both .js and .jsx extensions fix: ensure typescript resolution considers both .ts and .tsx extensions Feb 26, 2022
@bluwy
Copy link
Member

bluwy commented Feb 26, 2022

@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.

bluwy
bluwy previously approved these changes Feb 26, 2022
View reviewed changes
Copy link
Member

@bluwy bluwy left a 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.

bluwy
bluwy previously approved these changes Feb 27, 2022
View reviewed changes
aleclarson
aleclarson previously approved these changes Feb 27, 2022
View reviewed changes
@patak-dev patak-dev added this to the 2.9 milestone Feb 27, 2022
aleclarson
aleclarson approved these changes Feb 27, 2022
View reviewed changes
Copy link
Member

@aleclarson aleclarson left a 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.

leebeydoun
leebeydoun commented Feb 27, 2022
View reviewed changes
packages/vite/src/node/utils.ts
paths.push(name + type.replace('js', 'tsx') + query)
}
return 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.

This is really nice, thanks for improving the implementation I came up with.

@aleclarson aleclarson changed the title fix: ensure typescript resolution considers both .ts and .tsx extensions fix(resolve): try .tsx extension for .js import from typescript module Mar 2, 2022
@aleclarson aleclarson merged commit 72b8cb6 into vitejs:main Mar 2, 2022
@NMinhNguyen
Copy link
Contributor

Btw just curious, should .jsx map to .tsx or should only .js map to .tsx? I'm asking this because I'm wondering what TypeScript / Node ESM would expect. Because I thought the intention was for it to be .js -> .ts / .tsx, but I could be wrong.

@aleclarson
Copy link
Member

I think the .jsx => .tsx mapping is Vite-specific, not according to any standard. But not 100% on that.

@NMinhNguyen
Copy link
Contributor

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

@aleclarson
Copy link
Member

Looks like TypeScript emits .jsx files when jsx: preserve is used
https://www.typescriptlang.org/docs/handbook/jsx.html

image

@bluwy bluwy mentioned this pull request Mar 9, 2022
Closed
7 tasks
@wight554 wight554 mentioned this pull request Apr 26, 2022
Closed
7 tasks
@karlhorky karlhorky mentioned this pull request Feb 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleclarson aleclarson aleclarson approved these changes

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Team Board
Archived in project
Milestone
2.9
Development

Successfully merging this pull request may close these issues.

TypeScript source resolution does not work with jsx
5 participants
@leebeydoun @bluwy @NMinhNguyen @aleclarson @patak-dev