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

Doesn't work with vite 5.3 #1306

Closed
cranelee opened this issue Jun 18, 2024 · 11 comments · Fixed by #1312
Closed

Doesn't work with vite 5.3 #1306

cranelee opened this issue Jun 18, 2024 · 11 comments · Fixed by #1312
Labels
Priority An issue that will be prioritized by the maintainers

Comments

@cranelee
Copy link

Describe the bug

It works fine with sveltKit1/vite4. But I encounter some problems when I upgrade to svelteKit2.

❌ Encountered error in src/lib/components/mobile/home/mobileHomeHeader.svelte
Cannot read properties of undefined (reading 'watchFiles')

Reproduction

No response

@jycouet
Copy link
Contributor

jycouet commented Jun 18, 2024

It seems to be a vite 5.3.1 issue, going back to vite 5.2.13 looks good.
People are speaking about it here: https://discord.com/channels/1024421016405016718/1252330494088056855

@cranelee
Copy link
Author

@jycouet thanks, it still not working.
The server error is fragment can only take fragment documents. Found: undefined

@cranelee
Copy link
Author

@jycouet It works, I didn't remove the letter ^ before version at first time. Thanks a lot.

@SeppahBaws
Copy link
Collaborator

after some investigating: the error seems to be coming from here:

await find_graphql(page.config, parsed, {

which throws this error:

TypeError: Cannot read properties of undefined (reading 'watchFiles')
  at Object.addWatchFile (file:///C:/dev/houdini/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-BcXSligG.js:49663:21)
  at Object.addWatchFile [as dependency] (file:///C:/dev/houdini/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-BcXSligG.js:49815:11)
  at Object.enter (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89150:26)
  at AsyncWalker.visit (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:88994:26)
  at AsyncWalker.visit (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89025:22)
  at async AsyncWalker.visit (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89025:11)
  at async AsyncWalker.visit (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89025:11)
  at async AsyncWalker.visit (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89019:20)
  at async asyncWalk (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89053:10)
  at async find_graphql (file:///C:/dev/houdini/packages/houdini-svelte/build/plugin-esm/index.js:89056:3)

(doing the testing on my svelte-5 branch because it's the only one which has been upgraded to vite 5 so far...)

@SeppahBaws
Copy link
Collaborator

more investigating: (and mainly writing this down so that I don't forget)

When houdini-svelte looks for inline queries, it walks the AST.
In the walker's enter node, there is a function call to page.watch_file.

This page.watch_file is filled in the plugin setup code, correctly according to the docs

watch_file: this.addWatchFile,

Rollup docs (Vite adopts these): https://rollupjs.org/plugin-development/#plugin-context

By the time that the AST walker comes around to the call to page.watch_file, it goes into this code in Vite:
https://github.com/vitejs/vite/blob/cafa7d56f35ab0d55da732f16aa39569f31c581e/packages/vite/src/node/server/pluginContainer.ts#L787-L794

Which then in turn goes to:
https://github.com/vitejs/vite/blob/cafa7d56f35ab0d55da732f16aa39569f31c581e/packages/vite/src/node/server/pluginContainer.ts#L597-L605

where for some reason this._container is undefined, hence causing the Cannot read properties of undefined (reading 'watchFiles')` error.

Seems to be a side-effect of the Vite plugin infrastructure rewrite in 5.3.0 -> vitejs/vite#17288

net7toulouse pushed a commit to inp-net/churros that referenced this issue Jun 29, 2024
@macmillen
Copy link
Contributor

@SeppahBaws so is this an issue that needs to be fixed on Houdini's or Vite's side? And if Vite's side - is there already an open issue?

@SeppahBaws
Copy link
Collaborator

@SeppahBaws so is this an issue that needs to be fixed on Houdini's or Vite's side? And if Vite's side - is there already an open issue?

I'm not sure, I haven't worked with vite plugin development before, but since their release didn't mention any breaking changes I am inclined to think that it's something to be fixed on Vite's side?

I might open up an issue there anyways to ask for help with fixing this if we can't figure it out on our side...

We'll also look at adding houdini to their vite-ecosystem CI pipeline so that any breaking changes are detected before a release is pushed 🙂

@macmillen
Copy link
Contributor

I might open up an issue there anyways to ask for help with fixing this if we can't figure it out on our side...

We'll also look at adding houdini to their vite-ecosystem CI pipeline so that any breaking changes are detected before a release is pushed 🙂

Sounds like a great idea, thank you 🙂

@SeppahBaws SeppahBaws changed the title doesn't work with svelteKit2/vite 5 Doesn't work with vite 5.3 Jul 2, 2024
@SeppahBaws SeppahBaws pinned this issue Jul 2, 2024
@SeppahBaws SeppahBaws added the Priority An issue that will be prioritized by the maintainers label Jul 2, 2024
@antfu
Copy link

antfu commented Jul 2, 2024

It seems an edge case bug that bring in by the side-effect for refactoring the plugin container to a class. While should be fixed by watch_file: this.addWatchFile.bind(this), I agree this is more like a regression on Vite side, as Rollup doesn't require doing this.

@SeppahBaws
Copy link
Collaborator

It seems an edge case bug that bring in by the side-effect for refactoring the plugin container to a class. While should be fixed by watch_file: this.addWatchFile.bind(this), I agree this is more like a regression on Vite side, as Rollup doesn't require doing this.

Ah thanks for the reply!
I had a conversation in Discord with @dominikg who wasn't entirely sure if it's a good idea to store the addWatchFile function since it is context sensitive. @antfu what's your insight into this?

@antfu
Copy link

antfu commented Jul 2, 2024

After a brief discussion with the team, I think we made the consensus that this is considered an implementation detail, moving back would require us to do a lot manually .bind which would be costly on performance. In that sense, we will probably would not fix it. Also a bit context here (vitejs/vite#15610 (comment)). In the meantime, we will try to improve our docs to make this clear.

I would suggest this plugin to:

  • Use watch_file: this.addWatchFile.bind(this) for a quick fix
  • In the long term, consider refactoring your code to avoid saving addWatchFile function to global as it's contextual, and always prefer using this.addWatchFile with the current context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority An issue that will be prioritized by the maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants