feat(imports): support for import attributes in Vite

[redfox-mx]

Description

Fixes #14674, #12140

Adds support for import attributes by parsing them in the import analysis plugin.

Additional context

How this works:

• es-module-lexer provides information about where the attributes are. We already use this to clip them off.
• We grab the string (import attributes into source code) and convert to object literal { foo: 'bar' }
• Pass this through to resolveId where plugins can do what they want.

Build phase

• At build time we remove strip import attributes step since rollup remove it for us, it allows to pass attributes to resolveId hook

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

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

• Read the Contributing Guidelines, especially the Pull Request Guidelines.
• 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 ).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

PS: this pr solve some errors of #15654

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[redfox-mx]

Hi! lint job fails since import attribute syntax is not supported by eslint

[fregante]

Hi! lint job fails since import attribute syntax is not supported by eslint

That's weird, the ESLint config is already using the TypeScript parser, which does support import attributes.

https://github.com/redfox-mx/vite/blob/1f2d0f60cf008fb0dbba7bbf34c1b1d96cbfd7db/eslint.config.js#L36

See: https://x.com/bradzacher/status/1772901399518408775

I just used it myself, without any config: https://github.com/refined-github/shorten-repo-url/pull/48/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R34

[redfox-mx]

That's weird, the ESLint config is already using the TypeScript parser, which does support import attributes.
https://github.com/redfox-mx/vite/blob/1f2d0f60cf008fb0dbba7bbf34c1b1d96cbfd7db/eslint.config.js#L36

I know! But i dont really know how fix it. I get this message if I remove it form ignores option:

D:\repos\vite\playground\resolve\import-attributes.js
  1:74  error  Parsing error: ';' expected

BTW, no matter if I add or not ';', it doesn't desappear. Maybe it is caused by pnpm resolution to typescript 5.2.2 and import attributes are added in 5.3

vite/pnpm-lock.yaml

Lines 133 to 135 in 61357f6

	typescript:
	specifier: ^5.2.2
	version: 5.2.2

[redfox-mx]

@fregante yep! Typescript supports import attributes in v5.3

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html

[fregante]

I just realized:

At build time we remove strip import attributes step since rollup remove it for us

That's not what I'd want though, if the import attributes are removed they will be broken. I'm using Vite to bundle a library that imports a third-party JSON file.

• Don't bundle github-reserved-names refined-github/github-url-detection#194

Here I'm using Vite for tree-shaking, without bundling external dependencies.

[redfox-mx]

Well, import attribute removal is not about vite (only), rollup output removes attributes no matter what you do https://stackblitz.com/edit/rollup-import-attributes?file=rollup.config.js

In that sense, I updated the importAnalysisBuild plugin to "prevent" removing attributes from the source code and allow it to pass through the plugin system (resolveId).

NOTE: Remember, vite uses esbuild for development, but at build time, rollup bundles our code. If you want a transpiler option you can opt by esbuild, whin bundle option https://esbuild.github.io/getting-started/#bundling-for-the-browser

[fregante]

Thanks @redfox-mx! That mostly worked for me

[redfox-mx]

Hi @sheremet-va, nice to meet you. I update this pr to support import atributes into vite system, now covers hooks (not only virtual modules) and module graph integration.

I tried to write a test to cover import attributes proposal and read more about how @bluwy propose changes in #12140, so now import attributes metadata is preserved into module node.

Have a nice day n.n

[redfox-mx]

Maybe I don't really know if it is part of the plans for the vite team.

So, I close this PR. Maybe in the future when environment API or vite 6 will be ready. Idk

[sheremet-va]

Maybe I don't really know if it is part of the plans for the vite team.

I am actually invested in supporting this, I've bumped the priority a bit higher to discuss this in the next team meeting

[hi-ogawa]

I also wanted to bring this up and the PR seems nice. Thanks @redfox-mx for the effort!

One thing I was curious about is, what would happen when there are multiple imports for the same file but with different import attirbutes like this?

import repro1 from "./repro.js" with { custom: "x" };
import repro2 from "./repro.js" with { custom: "y" };

I was checking rollup and it shows a warning (rollup repl):

Module "src/main.js" tried to import "./repro.js" with "custom": "y" attributes, but it was already imported elsewhere with "custom": "x" attributes. Please ensure that import attributes for the same module are always consistent.

From the spec (especially since the change of tc39/proposal-import-attributes#131 about the cache key), it looks like this is allowed and it's more like a bundler side limitation. With vite/rollup plugin, it feels somewhat natural for people to try to transform differently based on import attributes even for the same file.

Though this might not be an issue in practice, I wanted to check what other thinks about this use case.

[bluwy]

That's one of the main reason why I'm conflicted with adding import attributes support in Vite currently. The right way is to allow specifying different attributes for the same import specifier, making it part of the cache key. But Rollup doesn't support that at the moment, I think it's a remnant from past import assertions support where assertions should always be the same for an import specifier. (And I think this is noted as a Rollup limitation somewhere). So if we support this in Vite now, it might not work in build.

[sheremet-va]

That's one of the main reason why I'm conflicted with adding import attributes support in Vite currently. The right way is to allow specifying different attributes for the same import specifier, making it part of the cache key. But Rollup doesn't support that at the moment, I think it's a remnant from past import assertions support where assertions should always be the same for an import specifier. (And I think this is noted as a Rollup limitation somewhere). So if we support this in Vite now, it might not work in build.

Yeah, if rollup doesn't support this, I am afraid we can't support it either 😞 I wonder if rolldown is better on this front.

[redfox-mx]

Hello 👋 thanks you for the feedback. I didn't think about use multiple import attributes for the same file. But maybe we can make it work with some "hack".

Currently (at least 4 months ago) vite drops import attributes at importAnalysis plugins (that was the first step to add "support") rewriting imports. So, in theory, We can rewrite import specifier to add a custom query that allows create a different module node to be resolved with no conflict. I mean

import repro1 from "./repro.js" with { custom: "x" };
import repro2 from "./repro.js" with { custom: "y", foo: "baz" };

Can be transformed to something like (at the same time we keep information into module graph)

import repro1 from "./repro.js?import-attribute=custom:x";
import repro2 from "./repro.js?import-attribute=custom:y,foo:baz";

Ps:
Support of the fully spec of import attributes from a bundler is not so easy since plugins can add/rewrite import attributes to any module. By other hand, rollup treat import attributes as "syntactic sugar" since at the end all imports attributes are deleted, there's no a really import attribute cause all your code is bundled.

Ps2: I'm still learning English, please be patient if I don't really explain me at all. 🙏

[redfox-mx]

Ps3: Happy viteconf!

[bluwy]

urrently (at least 4 months ago) vite drops import attributes at importAnalysis plugins (that was the first step to add "support") rewriting imports. So, in theory, We can rewrite import specifier to add a custom query that allows create a different module node to be resolved with no conflict.

I think this makes sense, and ideally we'd have to do something similar internally as the cache key too. But would this work properly with plugins receiving the attributes value from Vite and Rollup? For Rollup I wonder if we have to patch the hooks to preprocess the id (strip out internal attributes query) to kinda support it. If we can support that then I think we'd be most of the way there 🤔

[redfox-mx]

But would this work properly with plugins receiving the attributes value from Vite and Rollup?

Yes! In dev, if we add the import attributes into the ModuleNode all hooks can access to it through the PluginContainer. For build, rollup does it for us. Of course, since we drop import attributes from the importer source code we need to force resolution as importAnalysis does in dev to add attributes.

vite/packages/vite/src/node/plugins/importAnalysis.ts

Line 351 in f721677

const resolved = await this.resolve(url, importerFile)

*probabbly I need to investigate if the resolution change when we call this.resolve with the same id but with different attributes 🤔

For Rollup I wonder if we have to patch the hooks to preprocess the id (strip out internal attributes query) to kinda support it

Hummm I'm not really sure... I know, some rollup plugins rely on file extension (without query) like

if(!id.endsWith('.csv')) return;
// or
if(!(/.csv$/.test(id))) return;

But probabbly if You want to work with import attributes, You need to test attributes and not the id. Cause any file extension can be loaded as { type: 'csv' }.

Some use cases comes when you try to mix both types (in vite), for example:

import json from './foo.json?raw' with { custom: 'foo' };

In this case, plugin order controlls how code needs to be resolved, loaded, transformed. Ideally, Vite resolves foo.json, then it is loaded as string (if no 'pre' order is forced) and then it is transformed with a custom plugin with attributes { custom: 'foo' }.

About cache key, I assume that when we add a query into the specifier we create an Id, and it is our cache key.

import repro1 from "./repro.js?import-attribute=custom:x"; // 1
import repro2 from "./repro.js?import-attribute=custom:y,foo:baz"; // 2

// ... other file
import repro1 from "./repro.js?import-attribute=custom:x"; // 3

1 and 3 should have the same cache 🤔

[redfox-mx]

Import attributes is stage 4 now!!! 🎊

[bluwy]

For anyone following, we had discussed the required implementation a bit and sapphi opened an issue in Rollup (link above) that will be required by Vite in order to correctly implement the support.

[yyx990803]

For context: I opened an RFC for proper import attributes support #18534

The scope of impact is actually a bit larger than what this PR covers, but I think when we settle on the direction this PR can still be a good starting point.