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 plugins drop import attributes/assertions property only from src not node_modules #14674

Open
7 tasks done
arbassett opened this issue Oct 17, 2023 · 8 comments
Open
7 tasks done
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@arbassett
Copy link
Contributor

arbassett commented Oct 17, 2023

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 #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

> rollup-3.29.4@1.0.0 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

> rollup-4.1.4@1.0.0 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

> vite-4.11.4@1.0.0 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/rollup@3.29.4/node_modules/rollup/dist/es/shared/node-entry.js:2312:30)
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/rollup@3.29.4/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/rollup@3.29.4/node_modules/rollup/dist/es/shared/node-entry.js:25569:40)

> vite-5.0.0-beta.8@1.0.0 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/rollup@3.29.4/node_modules/rollup/dist/es/shared/node-entry.js:2312:30)
    at error (/home/arbassett/vite-attributes-assert-issue/node_modules/.pnpm/rollup@3.29.4/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/rollup@3.29.4/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  vite-4.11.4@1.0.0 build: `vite build`
Exit status 1

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

Validations

@arbassett
Copy link
Contributor Author

arbassett commented Oct 17, 2023

⁸As i was writing this 5.0.0-beta.10 landed with rollup v4 (#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 arbassett changed the title Vite plugins drop import attributes and assertions from project code Vite plugins drop import attributes/assertions property only from project code Oct 17, 2023
@arbassett arbassett changed the title Vite plugins drop import attributes/assertions property only from project code Vite plugins drop import attributes/assertions property only from src not node_modules Nov 16, 2023
@matthewp
Copy link
Contributor

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:

imports.map(async (importSpecifier, index) => {

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
Copy link
Contributor

Ah, it looks like es-module-parser might already support them. guybedford/es-module-lexer#150

@matthewp
Copy link
Contributor

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

@matthewp
Copy link
Contributor

Pull request is up: #15654

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) and removed pending triage labels Apr 29, 2024
@silverwind
Copy link

silverwind commented May 22, 2024

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.

@fregante
Copy link

fregante commented Jun 1, 2024

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

@silverwind
Copy link

silverwind commented Jun 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants