vite icon indicating copy to clipboard operation
vite copied to clipboard

Vite plugins drop import attributes/assertions property only from src not node_modules

Open arbassett opened this issue 2 years ago • 6 comments

Describe the bug

Vite has had a few issues opened about plugins not being able to access import assertions and have been closed due to import assertions being depreciated for import attributes #12140 & #10633 . the problem is this issue is only for direct project code any import that goes through node_modules will have access to these assertions/attributes. while the same code ran directly through rollup (v3 and v4) will have the attributes/assertions in project code and node_modules

This can be seen in vite's playground/import-assertion. by adding a simple vite config to that playground that logs options.

import { defineConfig } from "vite";


export default defineConfig({
  plugins: [
    {
      name: "test",
      enforce: "pre",
      resolveId(source, importer, options) {
        console.log(source,importer, options)
      },
    }
  ]
})

when running vite build it will show that the assert in index.html will not have any assertions but in import-assertion-dep/index.js it will have the assertion

vite/modulepreload-polyfill /home/projects/github-fktasj/playground/import-assertion/index.html { assertions: {}, custom: {}, isEntry: false }
/home/projects/github-fktasj/playground/import-assertion/index.html?html-proxy&index=0.js /home/projects/github-fktasj/playground/import-assertion/index.html { assertions: {}, custom: {}, isEntry: false }
./data.json /home/projects/github-fktasj/playground/import-assertion/index.html?html-proxy&index=0.js { assertions: {}, custom: {}, isEntry: false }
@vitejs/test-import-assertion-dep /home/projects/github-fktasj/playground/import-assertion/index.html?html-proxy&index=0.js { assertions: {}, custom: {}, isEntry: false }
./data.json /home/projects/github-fktasj/node_modules/.pnpm/file+playground+import-assertion+import-assertion-dep/node_modules/@vitejs/test-import-assertion-dep/index.js { assertions: { type: 'json' }, custom: {}, isEntry: false }

import attributes will be coming in typescript 5.3 and are already in rollup v4 without this plugin authors won't be able to leverage this new feature. i have also tried this with https://github.com/vitejs/vite/pull/14508 and it also had the same issues

i have created a example repo with a plugin to check for these assertions/attributes. it has rollup 3.x 4.x vite 4.x and 5.0.0-beta.8 both, versions of rollup handle this properly and both versions of vite fail.

Reproduction

https://stackblitz.com/~/github.com/arbassett/vite-attributes-assert-issue

Steps to reproduce

run pnpm build

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (3) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
    pnpm: 8.6.12 - /usr/local/bin/pnpm

Used Package Manager

pnpm

Logs

Click to expand!
> pnpm -r --sequential --no-bail build

Scope: 4 of 5 workspace projects

> [email protected] build /home/arbassett/vite-attributes-assert-issue/rollup-3.29.4
> rollup -c


../base-code/assertion/main.js → ./dist/main...
assertion test-import-assertion-dep /home/arbassett/vite-attributes-assert-issue/base-code/assertion/main.js { client: 'true' }
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/base-code/assertion/main.js { type: 'json', client: 'true' }
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/file+base-code+test-import-assertion-dep/node_modules/test-import-assertion-dep/index.js { type: 'json', node_modules: 'true' }
created ./dist/main in 58ms

> [email protected] build /home/arbassett/vite-attributes-assert-issue/rollup-4.1.4
> rollup -c


../base-code/assertion/main.js → ./dist/main.assertion...
assertion test-import-assertion-dep /home/arbassett/vite-attributes-assert-issue/base-code/assertion/main.js { client: 'true' }
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/base-code/assertion/main.js { type: 'json', client: 'true' }
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/file+base-code+test-import-assertion-dep/node_modules/test-import-assertion-dep/index.js { type: 'json', node_modules: 'true' }
created ./dist/main.assertion in 133ms

../base-code/attributes/main.js → ./dist/main.attributes...
attributes test-import-attributes-dep /home/arbassett/vite-attributes-assert-issue/base-code/attributes/main.js { client: 'true' }
attributes ./data.json /home/arbassett/vite-attributes-assert-issue/base-code/attributes/main.js { type: 'json', client: 'true' }
attributes ./data.json /home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/file+base-code+test-import-attributes-dep/node_modules/test-import-assertion-dep/index.js { type: 'json', node_modules: 'true' }
created ./dist/main.attributes in 40ms

> [email protected] build /home/arbassett/vite-attributes-assert-issue/vite-4.11.4
> vite build

vite v4.4.11 building for production...
transforming (1) index.htmlassertion test-import-assertion-dep /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js {}
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js {}
✓ 3 modules transformed.
✓ built in 214ms
[commonjs--resolver] import test-import-assertion-dep in /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js does not have client prop
error during build:
RollupError: import test-import-assertion-dep in /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js does not have client prop
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:2312:30)
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:25376:20)
    at resolveId (/home/arbassett/vite-attributes-assert-issue/vite-4.11.4/vite.config.ts.timestamp-1697564543843-4c50e158726058.mjs:52:14)
    at runHook/< (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:25569:40)

> [email protected] build /home/arbassett/vite-attributes-assert-issue/vite-5.0.0-beta.8
> vite build

vite v5.0.0-beta.8 building for production...
transforming (1) index.htmlassertion test-import-assertion-dep /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js {}
assertion ./data.json /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js {}
✓ 3 modules transformed.
✓ built in 174ms
[commonjs--resolver] import test-import-assertion-dep in /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js does not have client prop
error during build:
RollupError: import test-import-assertion-dep in /home/arbassett/vite-attributes-assert-issue/base-code/assertion/index.html?html-proxy&index=0.js does not have client prop
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:2312:30)
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:25376:20)
    at resolveId (/home/arbassett/vite-attributes-assert-issue/vite-5.0.0-beta.8/vite.config.ts.timestamp-1697564546917-e0c2406065dbd.mjs:52:14)
    at runHook/< (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:25569:40)
 ERR_PNPM_RECURSIVE_FAIL  


Summary: 2 fails, 2 passes

/home/arbassett/vite-attributes-assert-issue/vite-4.11.4:
 ERROR  [email protected] build: `vite build`
Exit status 1

/home/arbassett/vite-attributes-assert-issue/vite-5.0.0-beta.8:
 ERROR  [email protected] build: `vite build`
Exit status 1
 ELIFECYCLE  Command failed with exit code 1.

Validations

arbassett avatar Oct 17 '23 18:10 arbassett

⁸As i was writing this 5.0.0-beta.10 landed with rollup v4 (https://github.com/vitejs/vite/pull/14508) i updated the example to include it. The issue still persists

also rollup v4 will transform existing assert props to the attribute property for backwards compatibility

arbassett avatar Oct 17 '23 18:10 arbassett

I dug into this a little bit and I think the problem is that Vite doesn't always use Rollup's resolveId but instead uses es-module-lexer some of the times (what conditions I don't yet understand). That happens here: https://github.com/vitejs/vite/blob/06494443194763d6dd125740961cec82dd9a29d6/packages/vite/src/node/plugins/importAnalysis.ts#L421

Which means that we would need es-module-lexer to support them. Does this sound right @patak-dev @bluwy ? I'd be willing to poke at es-module-lexer and see what it takes to add.

matthewp avatar Jan 19 '24 01:01 matthewp

Ah, it looks like es-module-parser might already support them. https://github.com/guybedford/es-module-lexer/pull/150

matthewp avatar Jan 19 '24 01:01 matthewp

I have a spike of this being added to resolveId options.attributes using the information from es-module-lexer.

matthewp avatar Jan 19 '24 01:01 matthewp

Pull request is up: https://github.com/vitejs/vite/pull/15654

matthewp avatar Jan 19 '24 12:01 matthewp

I noticed is that import attributes are incorrectly removed when build.target is esnext or node18, e.g.:

import tlds from "tlds" with {type: "json"};

becomes

import tlds from "tlds";

I'm builing a node lib in vite and import attributes must be preserved in the compiled output as otherwise node can not load json modules in ESM. Running esbuild alone with the esnext target correctly preserves the attributes, so I assume this is a vite bug.

silverwind avatar May 22 '24 09:05 silverwind

Here's the related esbuild issue which confirms it should be working with target: 'node21'

  • https://github.com/evanw/esbuild/issues/3778

fregante avatar Jun 01 '24 04:06 fregante

Here's the related esbuild issue which confirms it should be working with target: 'node21'

* [Import attributes should be enabled for `node18` and >= `node20` targets evanw/esbuild#3778](https://github.com/evanw/esbuild/issues/3778)

Yeah, I assume this specific bug will be fixed with the next vite release that upgrades esbuild to 0.21.x. So no, it was not a vite bug as I initially assumed.

silverwind avatar Jun 01 '24 14:06 silverwind

Yeah, I assume this specific bug will be fixed with the next vite release that upgrades esbuild to 0.21.x. So no, it was not a vite bug as I initially assumed.

Actually, vite removes import attributes

https://github.com/vitejs/vite/blob/3af02bde7e7f26889d5c0eb300219c43ed1293ad/packages/vite/src/node/plugins/importAnalysis.ts#L494-L497

I have been working on a new PR based on @matthewp work to fix errors.

@bluwy Would you matter check my PR (#17485) 👉👈?

redfox-mx avatar Jun 29 '24 08:06 redfox-mx