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

feat: support rolldown-vite #7509

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Feb 17, 2025

Description

Good thing is that Vitest works with vite-rolldown for the most part (99%)

  • Removed sveltekit example for now because pnpm didn't like vite override and I don't want to deal with it 👀
  • Issues:
    • vite-rolldown doesn't expose Rollup.Program (or Rolldown.Program?)
    • rolldown doesn't support using syntax - we are transpiling the code down to node18, but it still doesn't work (although we use the esbuild field 🤔 ) - the test is skipped for now
      • there is no way to change the transform plugin if experimental.enableNativePlugin is true (the default)
      • examples/lit decorators don't seem to work correctly:

target was modified to include ES2021 because useDefineForClassFields is set to false and oxc does not support transforming useDefineForClassFields=false for ES2022+ yet

  • ✅ vite-rolldown uses '' + import.meta.url when transforming new URL to opt-out of rolldown's transform (this is an easy regexp change). Vitest also changes this transform to make the URL closer to the one that will be resolved in the browser (in node.js import.meta.url refers to a file, but we need a server URL there)
  • ✅ does optimizeDeps.rollupOptions.resolve.alias work? Browser Mode aliases vue packages to CJS versions testing-library is CJS (and it should import the same version of test-utils), and also we want to have the ability to use string slots and string templates
  • ✅ seems like I see this message even if I didn't specify any esbuildOptions (we always check for an empty stderr and this breaks some tests):

You've set "optimizeDeps.esbuildOptions" in your config. This is deprecated and vite already use rollup to optimize packages. Please use "optimizeDeps.rollupOptions" instead.

The value of esbuildOptions is { preserveSymlinks: false } and the environment name is client 🤔

This comes probably from here:

https://github.com/rolldown/vite/blob/1eee1f56ee90a08d82f465900caea6f2a618d358/packages/vite/src/node/config.ts#L1139

  • ✅ styles in the UI seem to be completely broken when built with vite-rolldown
Screenshot 2025-02-17 at 16 07 07
  • source maps sometimes are not correct. See test/config/unhandled-rejections.test.ts - the column should be 42 (this MIGHT BE an issue with custom Vitest plugins, but it was not a problem before):
  ❯ setup-unhandled-rejections.ts:2:41
       1| export function setup() {
       2|   void new Promise((_, reject) => reject(new Error('intentional unhand…
        |                                         ^ (this is incorrect position)
        |                                          ^ (it should be here where new Error starts)
       3| }

Note

Seems like we always had "Re-optimizing dependencies because vite config has changed" because of incorrect usage of cacheDir

Copy link

netlify bot commented Feb 17, 2025

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit b1c2886
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67b34bd398522f00080e1680
😎 Deploy Preview https://deploy-preview-7509--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa
Copy link
Contributor

Good thing is that Vitest works with vite-rolldown for the most part (99%)

Awesome progress already! 🎉

rolldown doesn't support using syntax - we are transpiling the code down to node18, but it still doesn't work (although we use the esbuild field 🤔 ) - the test is skipped for now

  • ...
  • examples/lit decorators don't seem to work correctly:

These two are oxc transform issues and they are tracked in oxc-project/oxc#9168 and oxc-project/oxc#9129.

styles in the UI seem to be completely broken when built with vite-rolldown

Probably this is because of unocss plugin not working unocss/unocss#4403.

@sapphi-red
Copy link
Contributor

sapphi-red commented Feb 18, 2025

Great!

  • does optimizeDeps.rollupOptions.resolve.alias work? Browser Mode aliases vue packages to CJS versions testing-library is CJS (and it should import the same version of test-utils), and also we want to have the ability to use string slots and string templates

I've fixed that yesterday so maybe it'll work if you upgrade rolldown-vite.
rolldown/vite@1eee1f5

  • seems like I see this message even if I didn't specify any esbuildOptions (we always check for an empty stderr and this breaks some tests):

You've set "optimizeDeps.esbuildOptions" in your config. This is deprecated and vite already use rollup to optimize packages. Please use "optimizeDeps.rollupOptions" instead.

I'll take a look this one.

  • optimizer unexpectedly re-optimizes dependencies with a message:

Is it actually re-optimized? or is it just the message? Ah, ok, it noticed that this message is shown by the scanner.

@sapphi-red
Copy link
Contributor

I've fixed the things related to warnings.

I also took a look the sourcemap one. I found that the sourcemaps output by OXC are slightly different from esbuild (oxc-project/oxc#9187). But I'm not sure if that's the reason.
I extracted the result here

const code = `${codeDefinition}${transformed}\n}}`

and tried to reproduce it minimally by doing

// run.mjs
import 'source-map-support/register.js'
globalThis.__vite_ssr_exports__ = {}
await import('./run2.mjs')
globalThis.__vite_ssr_exports__.setup()

// ---------------------------
// run2.mjs
'use strict';function setup() {
	void new Promise((_, reject) => reject(new Error("intentional unhandled rejection")));
}
Object.defineProperty(__vite_ssr_exports__, "setup", { enumerable: true, configurable: true, get(){ return setup }});

//# sourceMappingSource=vite-node
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBQU8sU0FBUyxRQUFRO0FBQ3RCLE1BQUssSUFBSSxRQUFRLENBQUMsR0FBRyxXQUFXLE9BQU8sSUFBSSxNQUFNLG1DQUFtQztBQUNyRiIsIm5hbWVzIjpbXSwiaWdub3JlTGlzdCI6W10sInNvdXJjZXMiOlsic2V0dXAtdW5oYW5kbGVkLXJlamVjdGlvbnMudHMiXSwic291cmNlc0NvbnRlbnQiOlsiZXhwb3J0IGZ1bmN0aW9uIHNldHVwKCkge1xuICB2b2lkIG5ldyBQcm9taXNlKChfLCByZWplY3QpID0+IHJlamVjdChuZXcgRXJyb3IoJ2ludGVudGlvbmFsIHVuaGFuZGxlZCByZWplY3Rpb24nKSkpXG59XG4iXSwiZmlsZSI6IkQ6L2RvY3VtZW50cy9HaXRIdWIvdml0ZXN0L3Rlc3QvY29uZmlnL2ZpeHR1cmVzL3VuaGFuZGxlZC1yZWplY3Rpb25zL3NldHVwLXVuaGFuZGxlZC1yZWplY3Rpb25zLnRzIn0=

But the error message showed setup-unhandled-rejections.ts:2:42.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 18, 2025

Ah, I should've mentioned earlier. Source map for global setup is not supported due to Vitest side #7101. The current assertion setup-unhandled-rejections.ts:2:42 works because transform output miraculously matched original source.

@sapphi-red
Copy link
Contributor

Ah, I see.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 18, 2025

@sheremet-va How did you run this PR locally? If I added rolldown-vite overrides, some dts build is failing, for example:

$ pnpm -C packages/vite-node build
src/source-map.ts(36,37): error TS7006: Parameter 'source' implicitly has an 'any' type.

[!] (plugin dts) RollupError: [plugin dts] src/source-map.ts: Failed to compile. Check the logs above.
src/source-map.ts

(The odd thing is that I don't see type error red squiggles on vscode and I have no idea what's the issue)

@sheremet-va
Copy link
Member Author

@sheremet-va How did you run this PR locally? If I added rolldown-vite overrides, some dts build is failing, for example:

$ pnpm -C packages/vite-node build

src/source-map.ts(36,37): error TS7006: Parameter 'source' implicitly has an 'any' type.



[!] (plugin dts) RollupError: [plugin dts] src/source-map.ts: Failed to compile. Check the logs above.

src/source-map.ts

(The odd thing is that I don't see type error red squiggles on vscode and I have no idea what's the issue)

I added link: override manually to overrides in the top level package.json

Did you build vite package?

@hi-ogawa
Copy link
Contributor

Hmm, I was using file: and that was failing. Switched to link: and now it's working on Vitest 👍 I can continue with this, but it might come back when using released package 🤔

@sheremet-va
Copy link
Member Author

Ah, I should've mentioned earlier. Source map for global setup is not supported due to Vitest side #7101. The current assertion setup-unhandled-rejections.ts:2:42 works because transform output miraculously matched original source.

There is another difference with source maps. I am not sure if it's correct or not, I think it is caused by a difference in boundaries. This code previously had a different column:

test.each([1, 2])('custom %s', () => {
//                ^ previously returned this column
//               ^ now returns this column
})

Raw JavaScript returns the column that Rolldown returns. We had to do column-1 to get the correct column here, now this code is gone.

@sheremet-va
Copy link
Member Author

sheremet-va commented Feb 18, 2025

I've fixed that yesterday so maybe it'll work if you upgrade rolldown-vite.

I used the same version yesterday. Still doesn't work (tried again today). I also tried ^@vue/test-utils$ alias, but it still doesn't seem to work. I am testing in this repo - https://github.com/vitest-dev/vitest-browser-vue (the last test expects template to work)

@sapphi-red
Copy link
Contributor

I've fixed that yesterday so maybe it'll work if you upgrade rolldown-vite.

I used the same version yesterday. Still doesn't work (tried again today). I also tried ^@vue/test-utils$ alias, but it still doesn't seem to work. I am testing in this repo - https://github.com/vitest-dev/vitest-browser-vue (the last test expects template to work)

I think the value needs to be a resolved path instead of a unresolved specifier here.
https://github.com/vitest-dev/vitest/pull/7509/files#diff-929cb96abb1c2e297c6711af0875db4ecdc700c54b824cc4ce7e0716657b28aeR537-R543
But given that the importer is not static, I guess it needs to be a plugin like it was in esbuild.
It's confusing that resolve.alias of rolldown works differently from vite or plugin-alias's resolve.alias. I'll make a feedback to them.

@sapphi-red
Copy link
Contributor

Ah, no, it's not that resolve.alias working differently, it's because Vite's internal plugin runs before resolve.alias. I think the workaround for now would be to be using a plugin instead as the custom plugins runs before the internal plugin.

@sheremet-va
Copy link
Member Author

Ah, no, it's not that resolve.alias working differently, it's because Vite's internal plugin runs before resolve.alias. I think the workaround for now would be to be using a plugin instead as the custom plugins runs before the internal plugin.

A rolldown plugin seems to work correctly.

@sapphi-red
Copy link
Contributor

I merged rolldown/vite#82 now, so the UI should be working now.

@sheremet-va
Copy link
Member Author

I merged rolldown/vite#82 now, so the UI should be working now.

Can confirm it does work now:

Screenshot 2025-02-19 at 15 08 52

@sheremet-va
Copy link
Member Author

sheremet-va commented Feb 19, 2025

I merged rolldown/vite#82 now, so the UI should be working now.

Is there a way to specify rolldown-vite as a dependency in this PR so CI can run with it? Ok, I see it supports pkg.pr.new

@sheremet-va
Copy link
Member Author

sheremet-va commented Feb 19, 2025

@sheremet-va How did you run this PR locally? If I added rolldown-vite overrides, some dts build is failing, for example:

$ pnpm -C packages/vite-node build
src/source-map.ts(36,37): error TS7006: Parameter 'source' implicitly has an 'any' type.

[!] (plugin dts) RollupError: [plugin dts] src/source-map.ts: Failed to compile. Check the logs above.
src/source-map.ts

(The odd thing is that I don't see type error red squiggles on vscode and I have no idea what's the issue)

Now we get the same error with pkg.pr.new 😄 Seems like it doesn't pick up rolldown types 🤔

@sapphi-red
Copy link
Contributor

vite-rolldown doesn't expose Rollup.Program (or Rolldown.Program?)

It seems Rollup.Program was not meant to be public, but was import-able due to microsoft/TypeScript#38592
https://github.com/rollup/rollup/blob/8f667b7c15b176728449a4917cb29fe5ee3a1c0c/src/rollup/types.d.ts#L1049

  • if experimental.enableNativePlugin is true (the default)

That option is disable by default. rolldown-vite uses OXC regardless of that option.

@sheremet-va
Copy link
Member Author

It seems Rollup.Program was not meant to be public, but was import-able due to

How do you type the parse function then 🤔

@sheremet-va
Copy link
Member Author

That option is disable by default. rolldown-vite uses OXC regardless of that option.

From what I can see in the code, the transform plugin doesn’t accept any options 🤔 JSDoc also says it’s true by default

@sapphi-red
Copy link
Contributor

It seems Rollup.Program was not meant to be public, but was import-able due to

How do you type the parse function then 🤔

I'm not sure, just that the export is not written there. It could be not intended. You can use ReturnType<typeof this.parse> (it's hacky though).

That option is disable by default. rolldown-vite uses OXC regardless of that option.

From what I can see in the code, the transform plugin doesn’t accept any options 🤔 JSDoc also says it’s true by default

Ah, the JSDoc is incorrect. To clarify, you can pass an option via config.oxc to OXC tranform plugin, but it won't unblock you.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 20, 2025

Now we get the same error with pkg.pr.new 😄 Seems like it doesn't pick up rolldown types 🤔

Slight progress. I added "rolldown": "1.0.0-beta.3-commit.b546e53" (the same one used by current rolldown-vite) to root and pnpm -C packages/vite-node build got through. Now it's failing with pnpm -C packages/browser/ build:node but this looks like there are duplicate vite somewhere.


I pushed my hack and build passed on CI 😀

@@ -12,5 +12,6 @@ test('unhandled rejections of main thread are reported even when no reporter is
expect(exitCode).toBe(1)
expect(stderr).toContain('Unhandled Rejection')
expect(stderr).toContain('Error: intentional unhandled rejection')
// FIXME: this should be the correct location, but rolldown (possibly) shows 2:41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is also alright. It should be fixed once we figure out global setup source map #7101.
While esbuild preserved column exactly, oxc replaced two spaces with one tab, so the transformed output became one character off from the original one.

source map comparison

image

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 21, 2025

Now we get the same error with pkg.pr.new 😄 Seems like it doesn't pick up rolldown types 🤔

Slight progress. I added "rolldown": "1.0.0-beta.3-commit.b546e53" (the same one used by current rolldown-vite) to root and pnpm -C packages/vite-node build got through. Now it's failing with pnpm -C packages/browser/ build:node but this looks like there are duplicate vite somewhere.

I pushed my hack and build passed on CI 😀

I found the culprit of dts build error. This is because rollup-plugin-dts sets compilerOptions.preserveSymlinks: true by default https://github.com/Swatinem/rollup-plugin-dts/blob/deddf70ccc09345f41b3129004ce3321b0529a54/src/program.ts#L25-L26. This likely requires externalized (even transitively) dep to be visible directly from project, so rolldown is required to be in the root. The same error can be seen on IDE by setting preserveSymlinks: true inside our tsconfig.json.

While I have mostly same setup for rolldown-vite testing PR on my plugin hi-ogawa/vite-plugins#673 where I use rollup-plugin-dts through tsup, it turns out tsup overwrites such "likely bad" default back to preserveSymlinks: false https://github.com/egoist/tsup/blob/0328fd64a771adb0789968775c22b4317683fafe/src/rollup.ts#L141-L145, so people using tsup won't see this issue when switching from vite to rolldown-vite.

Here is a minimal repro of the issue https://github.com/hi-ogawa/reproductions/tree/main/rolldown-vite-rollup-plugin-dts.

@sheremet-va
Copy link
Member Author

So we don't see this issue in main because we have rollup in dependencies and it makes it visible?

@hi-ogawa
Copy link
Contributor

So we don't see this issue in main because we have rollup in dependencies and it makes it visible?

Yes, I think that's the reason. It's not possible to reproduce the exact same scenario with rollup because rollup-plugin-dts obviously requires rollup to be installed in root 🙃

For our case, I think either we can install rolldown in root or manually add dts({ compilerOptions: { preserveSymlinks: false } }).

@sheremet-va
Copy link
Member Author

sheremet-va commented Feb 21, 2025

Seems like all tests are passing, except:

And that's it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants