mdx-js / mdx

Markdown for the component era
https://mdxjs.com
MIT License
17.76k stars 1.14k forks source link

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

Closed aaronadamsCA closed 3 months ago

aaronadamsCA commented 3 months ago

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: https://github.com/vitejs/vite/pull/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 commented 3 months ago

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?

wooorm commented 3 months ago

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

remcohaszing commented 3 months ago

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

wooorm commented 3 months ago

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

wooorm commented 3 months ago

Like, the sveltekit stuff also was done in vite: https://github.com/vitejs/vite/pull/8969

remcohaszing commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/evanw/esbuild/commit/cc2561421a450551b8ad6c96066b7e094e198003

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 commented 3 months ago

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 https://github.com/vitejs/vite/issues/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 commented 3 months ago

@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 commented 3 months ago

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 https://github.com/vitejs/vite/issues/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 commented 3 months ago

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 https://github.com/vitejs/vite/issues/8993.

bluwy commented 3 months ago

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.