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

feat(trace): resolve incorrect extension #9486

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

NicholasLYang
Copy link
Contributor

Description

Invalid extensions are unfortunately too common in imports. Therefore if we can't resolve with the extension, we strip it and try again.

Testing Instructions

Added a test where we try to import with the wrong extension

@NicholasLYang NicholasLYang requested a review from a team as a code owner November 21, 2024 17:58
Copy link

vercel bot commented Nov 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2024 5:58pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-gatsby-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-native-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-svelte-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-tailwind-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm
examples-vite-web ⬜️ Ignored (Inspect) Nov 21, 2024 5:58pm

Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

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

Two small questions

let type_package = format!("@types/{}", import);
let resolved_type_import = resolver
.resolve(file_path, type_package.as_str())
if !import.starts_with(".") {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a check against ./?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I don't think packages can start with .?

}

// Also try without the extension just in case the wrong extension is used
let without_extension = Utf8Path::new(import).with_extension("");
Copy link
Member

Choose a reason for hiding this comment

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

Just calling out that this will convert something like types.d.ts to types.d source, is that the desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I originally had an explicit check for types.d.ts that stripped it all the way back to types but when I ran this on the other repo, that didn't fix any errors. I suspect oxc-resolver also treats types.d as the name, so this should work just fine

@NicholasLYang NicholasLYang merged commit 0a2ef17 into main Nov 21, 2024
40 checks passed
@NicholasLYang NicholasLYang deleted the nicholasyang/resolve-incorrect-extension branch November 21, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants