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

[vite] Can't import TypeScript files using .js extension #2527

Closed
4 tasks done
aaronadamsCA opened this issue Aug 18, 2024 · 13 comments
Closed
4 tasks done

[vite] Can't import TypeScript files using .js extension #2527

aaronadamsCA opened this issue Aug 18, 2024 · 13 comments
Labels
🙉 open/needs-info This needs some more info 🤞 phase/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem

Comments

@aaronadamsCA
Copy link

aaronadamsCA commented Aug 18, 2024

Initial checklist

Affected packages and versions

3.0.1

Link to runnable example

https://codesandbox.io/p/github/aaronadamsCA/mdx-issues/file-extension?file=/src/mdx.mdx

Steps to reproduce

In a .mdx file, try importing a .ts or .tsx file using a .js extension.

This is specifically supported by Vite: vitejs/vite#5510

And it is the correct file extension to use when importing from TypeScript files, even in a noEmit environment: https://www.typescriptlang.org/docs/handbook/modules/theory.html

Expected behavior

The import should work using a .js extension.

Actual behavior

The import fails when using a .js extension.

image

[plugin:vite:import-analysis] Failed to resolve import "./items.js" from "src/mdx.mdx". Does the file exist?

You can work around this by importing using the .ts or .tsx extension (and setting "allowImportingTsExtensions": true in your tsconfig.json to quiet the corresponding warning), but this should work without this workaround.

To fix it, I think your Vite plugin needs to figure out how to tell Vite that options.isFromTsImporter should be true here:

https://github.com/vitejs/vite/blob/3b8f03d789ec3ef1a099c884759bd4e61b03ce7c/packages/vite/src/node/plugins/resolve.ts#L210-L220

It's entirely possible you'll need an upstream change for this though, I really have no idea. Just wanted to share my troubleshooting findings so far.

Runtime

No response

Package manager

No response

OS

No response

Build and bundle tools

No response

@remcohaszing
Copy link
Member

I think what you’re saying makes sense, but it requires further investigation.

Is is possible to import .ts / .tsx files using the .js extension from files using the .js extension?

@remcohaszing remcohaszing added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info 👶 semver/patch This is a backwards-compatible fix 🤞 phase/open Post is being triaged manually labels Aug 19, 2024
@wooorm
Copy link
Member

wooorm commented Aug 19, 2024

If the code to change is in Vite, why not raise the issue with Vite?

@remcohaszing
Copy link
Member

The code change is we set an option for Vite somewhere in our plugin. The OP just linked where Vite checks this option.

@wooorm
Copy link
Member

wooorm commented Aug 19, 2024

Where do we set Vite options? Why can Vite users not pass options to it?

@wooorm
Copy link
Member

wooorm commented Aug 19, 2024

Like, the sveltekit stuff also was done in vite: vitejs/vite#8969

@remcohaszing
Copy link
Member

Ok, so possibly mdx should be added to https://github.com/vitejs/vite/blob/v5.4.1/packages/vite/src/node/optimizer/scan.ts#L47. I’m not really sure what that regex means though. MDX is more JS(X)-like than HTML-like. Perhaps @patak-dev can give us some insight.

@patak-dev
Copy link

The other file types in the HTML-like regex are SFCs, where you have <script> elements that we want to scan for imports. Regarding the PR above, it fixed the scanner for svelte and other HTML like files. The error in the OP isn't from the scan phase though. @bluwy, does svelte creates a virtual module ending with .ts so the .js to .ts kicks in during normal transform? The MDX plugin would need to do something similar I imagine here. About the scan part, maybe it would be similar to how Astro works? Again, sorry @bluwy, does Astro injects a custom esbuild plugin for the scanning part?

@aaronadamsCA
Copy link
Author

aaronadamsCA commented Aug 19, 2024

This could totally be an upstream change! But I really wasn't sure, so I reported it here first.

I really don't mind the "bad" import in our files for now, we've had it worked around for a month or two. Just wanted to let you know before it causes a real problem.

In case it helps, I know esbuild fixed this here: evanw/esbuild@cc25614

But it felt like, given I'm using this as a Vite plugin, maybe this is Vite's job. I wasn't sure and I figured you'd know a lot better than me.

@bluwy
Copy link

bluwy commented Aug 20, 2024

I think this issue isn't related to scanning or prebundling, so we should probably focus on the path resolving part in Vite. But:

@bluwy, does svelte creates a virtual module ending with .ts so the .js to .ts kicks in during normal transform?

vite-plugin-svelte sets the lang here for Vite to detect.

About the scan part, maybe it would be similar to how Astro works? Again, sorry @bluwy, does Astro injects a custom esbuild plugin for the scanning part?

At the moment, Astro doesn't inject an esbuild plugin for the scanning part, but maybe it would in the future


In practice, Vite should probably resolve .js to .ts for any files part of the tsconfig, but currently it isn't which is a bug and tracked at vitejs/vite#8993

However, that issue hasn't been prioritized much recently since the introduction of moduleResolution: 'bundler' and allowImportingTsExtensions, and usually you should import the file path as is, like ./foo or ./foo.ts, not ./foo.js since bundlers don't actually build to ./foo.js at any point in time, and is an artifact of tsc.

I think it's fair if mdx doesn't want to set a vite specific option (but you also could if you think it's harmless, like the linked code above). Otherwise I think the proper solution is to not use .js imports and enable allowImportingTsExtensions.

@wooorm
Copy link
Member

wooorm commented Aug 20, 2024

@bluwy What are the values of meta.vite.lang? What does vite-plugin-svelte set, what does Vite accept?
I don’t mind setting it, to “javascript” or so, but there isn’t really a reason to set “typescript” there for MDX, as MDX knows nothing about typescript.

Also, context: we have a rollup plugin and we have an esbuild plugin. There’s no particular Vite support. If Vite now supports esbuild plugins, and esbuild supports this correctly, we can also start recommending using our esbuild plugin.

@bluwy
Copy link

bluwy commented Aug 20, 2024

Sorry, you'd set "ts" or "js" (like the file extensions). In your case, you'd likely unconditionally set meta.vite.lang: 'ts' explicitly so Vite can always resolve the .js to .ts thing (like astro).

That value is only used in Vite here, so if you set anything that's not ts/mts/cts/tsx, then Vite will see that it's not typescript. It doesn't error if you set some invalid values there.

but there isn’t really a reason to set “typescript” there for MDX, as MDX knows nothing about typescript.

That's true, but I think it's unlikely for Vite to expand on that to mean actually TS anytime soon. Right now it's only for the .js to .ts resolution, and if anything if vitejs/vite#8993 is one day resolved, we won't likely need that property anymore too.

Also, context: we have a rollup plugin and we have an esbuild plugin. There’s no particular Vite support. If Vite now supports esbuild plugins, and esbuild supports this correctly, we can also start recommending using our esbuild plugin.

Vite doesn't use esbuild for bundling so an esbuild plugin likely won't work. The rollup plugin is probably the best way still.

@aaronadamsCA
Copy link
Author

aaronadamsCA commented Aug 20, 2024

Based on the conversation, I'm think I'm sold that this is a Vite problem.

Their extension fallback only kicks in if the importer is a TypeScript file, but that logic seems backwards. The whole point of importing from a .js extension is that's where the file will be after bundling. So the logic should really be looking at whether the exporter is potentially a TypeScript file.

There's no reason an MDX file—or an HTML file, or any other type of file—shouldn't be able to import a .ts file using .js extension. And I don't think any plugin should need to do anything to enable that.

@bluwy - simultaneous post, sorry! Not sure if you'd agree with the above, but maybe it's a conversation worth continuing over at vitejs/vite#8993.

@bluwy
Copy link

bluwy commented Aug 20, 2024

I agree that Vite's implementation is backwards and we should fix that issue. But as mentioned, when you're using a bundler there's very less reason to use .js for .ts so it's not as prioritized now. But we can move that discussion there for sure if you have other thoughts about it.

@aaronadamsCA aaronadamsCA closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙉 open/needs-info This needs some more info 🤞 phase/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

No branches or pull requests

5 participants