sveltejs / vite-plugin-svelte

Svelte plugin for http://vitejs.dev/
MIT License
855 stars 104 forks source link

Vite dev mode breaks inertia/svelte #175

Closed burlesona closed 3 years ago

burlesona commented 3 years ago

Describe the bug

This is described in great detail here: https://github.com/inertiajs/inertia/issues/826

In short: Inertia/Svelte defines a $page store and expects that to be available in components: https://github.com/inertiajs/inertia/blob/master/packages/inertia-svelte/src/page.js

After Vite Build this works fine, but on Vite Dev the $page store is just an empty object {}.

For severity I put high because it essentially breaks the entire Inertia/Svelte framework. No arguments if you feel it should be lowered to medium.

I'm not 100% sure where the bug is originating, and hoping that asking around the related projects we can figure it out or get some hints.

Reproduction

Detailed steps to reproduce here: https://github.com/inertiajs/inertia/issues/826

Logs

Please see https://github.com/inertiajs/inertia/issues/826

System Info

System:
    OS: macOS 11.5.2
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 5.78 GB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.9.0 - /usr/local/bin/node
    Yarn: 1.22.11 - /usr/local/bin/yarn
    npm: 7.21.1 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 93.1.29.81
    Safari: 14.1.2
  npmPackages:
    @sveltejs/vite-plugin-svelte: ^1.0.0-next.11 => 1.0.0-next.22 
    svelte: ^3.37.0 => 3.42.4 
    vite: ^2.5.4 => 2.5.6

Severity

blocking all usage of vite-plugin-svelte

bluwy commented 3 years ago

The repro seems to be using an old version of vite-plugin-svelte, a lot of improvements has been made since then. It also uses PHP and I don't really have the experience to set that up unless there's an online environment to test quickly.

Nonetheless, if this happens for you in the latest version as well, I'd appreciate if you can bring an up-to-date minimal repro to further investigate this. I can't think of a reason why the error would appear now.

burlesona commented 3 years ago

@bluwy thanks for the reply. I'm using a Ruby backend, not PHP, but I'm assuming that for you a Node backend would make a better demo. That'll take a minute to set up, but I'll check back in when I have it running.

burlesona commented 3 years ago

Ok, I was able to find a small app I made a while ago and update it to show the same issue: https://github.com/burlesona/vite-svelte-inertia-bug

If you run this you'll see $page is {} in dev mode but populated correctly in prod builds.

dominikg commented 3 years ago

It looks like the initial value for page is {} so the question is when and where is that store written to and why doesn't it happen on dev.

It could also be another incarnation of the "duplicate svelte" problem we used to have, you could try to add inertia-svelte to optimizeDeps.exclude :thinking:

burlesona commented 3 years ago

Hi @dominikg, thanks for chiming in. When I try putting excluding inertia-svelte the page fails with the following error:

The requested module '/node_modules/lodash.isequal/index.js?v=70a89108' does not provide an export named 'default'

So unfortunately that doesn't fix it :/

bluwy commented 3 years ago

@burlesona I tried to run the repro earlier but couldn't get http://localhost:4000 to fetch http://localhost:3000/src/main.js. So I'm not seeing the real bug. I did however dig into @inertiajs/inertia-svelte but I don't see anything fishy with how the package is written and bundled.

To answer dominik's question, it seems that this is the only place where $store is updated (which $page derives from). And that piece is code is from Inertia.init, so I'm guessing that's where the source of the bug is, that Inertia itself is not playing well with Vite's dev server setup.

I don't see anywhere vite-plugin-svelte could be interfering though, so I'd say we should track the Inertia issue instead.

bluwy commented 3 years ago

Hi @burlesona. Any news about this on inertia side? I think the bug is within inertia itself (as explained above). If you think so too, we should close this issue and track it at https://github.com/inertiajs/inertia/issues/826 instead.

burlesona commented 3 years ago

@bluwy I haven't heard from them but I tend to agree that it's on the inertia side. The only thread I'm still considering on the vite side is what exactly optimizeDeps does. That seems to be the only thing that changes code between vite dev and vite prod, but since I'm not sure what it does it's not obvious to me whether that could realistically be causing this behavior.

If you have any insight on that I'd love to hear it :)

Meanwhile I agree it does not seem to be something specific to vita-plugin-svelte so I'll mark this as closed. Thanks for your help.

bluwy commented 3 years ago

Cool :+1:

The only thread I'm still considering on the vite side is what exactly optimizeDeps does

optimizeDeps is currently used for pre-bundling to support CJS dependencies in Vite for dev mode only. For vite-plugin-svelte, we do a bit of special treatment for Svelte dependencies. The topic is briefly discussed here, but for a more in-depth explanation of what we do, currently all Svelte dependencies (detected via pkg.svelte) are added to optimizeDeps.exclude by default, because we don't want to pre-bundle them per https://github.com/vitejs/vite/issues/3910. But on top of that, we also re-include its nested dependencies, so we add something like svelte-dep > nested-dep to optimizeDeps.include. This is so that nested CJS dependencies are optimized too, otherwise it won't work in Vite.

But with that said, I don't think it would affect Inertia in any way, unless (maybe) @inertiajs/inertia is added to optimizeDeps.exclude.

burlesona commented 3 years ago

@bluwy I hope you don't mind another follow up question as I'm just trying to figure this thing out...

Can you help me understand the difference between how Webpack require and Vite import work?

In the Inertia docs they give examples of how to setup pages assuming webpack:

import Layout from './Layout'

createInertiaApp({
  resolve: name => {
    const page = require(`./Pages/${name}`).default
    page.layout = page.layout || Layout
    return page
  },
  // ...
})

For this to work in Vite I had to change it to work like this:

  resolve(name){
    let n = name.split('/');
    if (n.length == 1) return await import(`./pages/${n[0]}.svelte`);
    if (n.length == 2) return await import(`./pages/${n[0]}/${n[1]}.svelte`);
    if (n.length == 3) return await import(`./pages/${n[0]}/${n[1]}/${n[2]}.svelte`);
  },

I know that's gross, I couldn't figure out any other way to allow the page directory to be nested and still have production builds work - the glob resolve thing won't do recursive subdirectories.

Anyway the above works fine, but I also notice that if I want to do what they suggest and set a default layout I tried to modify it further...

  async resolve(name){
    let n = name.split('/');
    let page;
    if (n.length == 1) page = await import(`./pages/${n[0]}.svelte`);
    if (n.length == 2) page = await import(`./pages/${n[0]}/${n[1]}.svelte`);
    if (n.length == 3) page = await import(`./pages/${n[0]}/${n[1]}/${n[2]}.svelte`);
    page.default.layout = DefaultLayout;
    return page;
  },

This however does not work. page.default does return the actual component object you would expect, and it's fine to set a property on this. But in the inertia docs they actually return the component object returned by .default and expect it to work, whereas I've found that you have to return the object returned by dynamic import or the whole thing breaks. If I set anything on page.default, it seems to disappear / do nothing.

Sorry this is fuzzy due to my poor understanding of the build system, and I don't know what context will be sufficient for you to understand my question. But in short, do you know if there's a significant difference in the webpack require and vite dynamic import that would change what is coming back from this resolve function?

The reason I'm interested is it seems that this all tracks back to Inertia's init and setPage functions not working correctly in dev mode, and that seems to be because they aren't able to set values on the page after it is resolved, just as I'm not able to do that in the resolve function I defined.

Sorry again to be asking you questions only partially related to your library, I'm just struggling with this and nobody else is really responding. Thanks for your generosity in answering questions!

bluwy commented 3 years ago

I don't quite follow with your issue here, I assume in your thrid code snippet, you have to return page.default? Re webpack require vs vite import, they're basically the same thing, except the former is CJS style and the latter is ESM style. The latter should be preferred these days, even in webpack. I'm assuming webpack's glob imports handle the dynamic variable differently by interpreting as /pages/**/*.svelte, whereas vite/rollup interprets as /pages/*/*/*.svelte. The latter takes the safer path so you don't accidentally over-bundle things.

If you want to explicitly bundle everything under a directory, you can use import.meta.glob. And using the name, you can find the correct import via the import keys. So something like:

const comps = import.meta.glob('./pages/**/*.svelte'')
const match = comps[`./pages/${name}.svelte`]
// continue

If you need more help, I'd suggest asking around in the Vite discord. There could be someone familiar with inertia as well.

burlesona commented 3 years ago

@bluwy thanks for the meta glob tip and all the feedback, I really appreciate it!