fix: emit assets from SSR build

[Rich-Harris]

Description

This closes #11429, by removing the code that causes the bug

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

I'm puzzled about why we didn't get a bug report before. I imagine the idea of ffaf50d was avoiding duplicating the generation of assets, as normally all of them will already be there from the client build as you explained. I don't know if we can optimize this given that the build process for the client and ssr is independent in Vite right now, and that the user may configure Vite to avoid wiping the dist folder. Your fix seems to be the safest choice. Let's check with ecosystem-ci first, and merge it if we are all green.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	✅ success
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
previewjs	✅ success
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[bluwy]

I don't know if we can optimize this given that the build process for the client and ssr is independent in Vite right now, and that the user may configure Vite to avoid wiping the dist folder. Your fix seems to be the safest choice. Let's check with ecosystem-ci first, and merge it if we are all green.

I don't quite understand this. Isn't it intentional to not emit assets in SSR builds? It's expected that the assets come from the client build. Otherwise both builds contain duplicated assets and that increases the final client+server bundle size.

This seems like a big change instead of a fix. Maybe we could add an experimental option for now until we have a plan for the caveat.

As a workaround, you can do something like this too. But I agree that this isn't nice.

[patak-dev]

@bluwy I'm assuming that most frameworks will have the assetFileNames be the same on SSR and the client build so what we are duplicating is writing the assets to disk, but not the assets themselves. But you are right about the change being too big for a patch. Let's add it to the 4.1 milestone and release it in January. @brillout @danielroe would you help us check this PR?

[Rich-Harris]

I don't quite understand this. Isn't it intentional to not emit assets in SSR builds?

The current behaviour comes from this (pre-v2) commit, which doesn't appear to be linked to a PR so the motivation is a mystery. But I think it's reasonable to suggest that it was just an oversight — the assumption was that you'd always import assets into modules that Vite sees during the non-SSR build, and that assumption turned out to be wrong.

The workaround you suggest is what I'm doing currently (taking care to make sure that the resulting imports are tree-shaken from the client bundle), and it's basically fine if a little undiscoverable. In the SvelteKit case it adds the constraint that assets can't be in $lib/server.

Given the availability of the workaround, 4.1 seems totally reasonable. Thanks!

[bluwy]

@bluwy I'm assuming that most frameworks will have the assetFileNames be the same on SSR and the client build so what we are duplicating is writing the assets to disk, but not the assets themselves

Ah so you're expecting that both the client and server builds the assets to the same directory. That could work if the user/metaframework sets it up, and we could document it too. Currently Vite's SSR setup recommendation would duplicate the assets by default with this PR, eg with pnpm create vite-extra --template ssr-vanilla.

[7antra]

Hello, I don't know if I'm off the topic here, but here is a situation that I hope could be helpful, maybe.

Before Sveltekit 1.0 (so Vite 4.0), if you imported a component from an external package (from your /node_module/external_package) and this component was importing assets like .svg from here own repo : in dev and from ssr you could read the "url" of the SVG like : /node_modules/external_lib/.../assets/test.svg and in production it was something like /client/immutable/assets/test-875jhg6.svg

Now, "external assets" seems hashed and import in client/immutable like before, but vite or svelting not resolve the path, and serve an "undefined" url for all assets coming from external package.

NB: even if your vite config is fs : { allow : ['../../'] }

(sorry for my english)

[brillout]

This is great. VPS goes to great length to work around the fact that SSR assets aren't emitted: https://vite-plugin-ssr.com/includeAssetsImportedByServer#page-content (its implementation is far from trivial, to say the least).

I'm 💯 for merging this.

[Rich-Harris]

@7antra that sounds unrelated to this change. I'd suggest opening a new issue with a minimal reproduction, ideally on this repo if you can reproduce it without SvelteKit, or on https://github.com/sveltejs/kit/issues if not

[benmccann]

@Rich-Harris it looks like you need to run pnpm format to fix the failing lint job

[Rich-Harris]

I did, and nothing changed 🤷

[patak-dev]

That's strange, it added the missing trailing comma for me.

[brillout]

This will emit the assets to dist/server/assets/ instead of dist/client/assets/, right? (Assuming dist/server/ is the value of config.build.outDir for ssr: true and dist/client/ the value for ssr: false.)

But the assets should be emitted to dist/client/assets/ instead. (Since static assets are served from dist/client/assets/.)

(As a tangent: this is one argument more in the growing list of reasons for not having two seperate builds. There really should be only a single $ vite build call instead of having to run it twice $ vite build && vite build --ssr.)

[Rich-Harris]

This will emit the assets to dist/server/assets/ instead of dist/client/assets/, right?

By default yes but in our case we use rollupOptions.output.assetFileNames and it's the same for both server and client builds; I assume any other framework in a similar position would do likewise.

(As a tangent: this is one argument more in the growing list of reasons for not having two seperate builds. There really should be only a single $ vite build call instead of having to run it twice $ vite build && vite build --ssr.)

Co-sign, though it can easily be more than two (we have a third build for service workers, for example, though we control the build config tightly for that — it doesn't use vite.config.js). One thing I'll note though is that in sveltejs/kit#8636 we're switching the order of the builds — server first, then client, so that the client route manifest can be written with knowledge of the server build to prevent unnecessary requests — and Vite's trademark flexibility is invaluable in those situations.

[patak-dev]

@Rich-Harris we discussed the PR in a team meeting and decided to move forward with an opt-in experimental option so we can get it in 4.1. We may review this option in Vite 5 if we find a better way but it is good if frameworks are able to experiment with it with Vite 4.

I think we should name this option build.ssrEmitAssets defaulting to false. I can add it to your PR if you would like before merging.

[benmccann]

By default yes but in our case we use rollupOptions.output.assetFileNames and it's the same for both server and client builds; I assume any other framework in a similar position would do likewise.

@Rich-Harris are you saying that's true today or something we'd implement in the future? While we do use that option today, it looks to me like the assets are sent to .svelte-kit/output/client/. We use the same assetFileNames, but that doesn't really matter since the outDir is different between client and server builds and sends the assets to different directories

[benmccann]

this change seems fine to me though it's not entirely clear to me how we'd use it on the SvelteKit side

[Rich-Harris]

are you saying that's true today

I thought it was, because of https://github.com/sveltejs/kit/blob/9e560183348b69996b13397001996961591544c6/packages/kit/src/exports/vite/index.js#L441, but prefix is just ${appDir}/immutable

it's not entirely clear to me how we'd use it on the SvelteKit side

We would need to copy the contents of .svelte-kit/output/server/_app to .svelte-kit/output/client/_app, which is a little clunky.

[benmccann]

Yeah, okay. That's what I was guessing. It's a bit clunky as you said, but a better user experience, so probably worth doing it. I don't know if we can skip the copy step by making assetFileNames start with ../client (I don't think it will work, but am only 50% certain). Or maybe cleaner would be to leave it in server but then have the adapter copy from both places. Anyway, as long as you're aware we'll need to do some extra step along those lines and are okay with it then this PR looks good to me.

[Rich-Harris]

I don't know if we can skip the copy step by making assetFileNames start with ../client

That won't work, I tried. The asset path, relative to outDir, needs to be the same for both server and client builds otherwise SSR'd HTML (think <img src={image}>) will point to non-existent URLs

[patak-dev]

I think we could still merge this PR, and you can check in SvelteKit how to best use the new option. If you end up discovering that this isn't the right way to go, we can deprecate it and remove it in Vite 5. I'll merge it before releasing 4.1-beta.1 tomorrow, except if I see you have other ideas.

[iMrDJAi]

@patak-dev @Rich-Harris Can you by any chance consider backporting this to Vite v3?

[patak-dev]

We don't usually backport features @iMrDJAi, is there any reason preventing you to update your project to Vite v4? 3 to 4 should normally be a painless update https://vitejs.dev/guide/migration.html

[iMrDJAi]

@patak-dev Upgrading to Vite v4 was much complicated than I expected, some dependencies are no longer functional. This is due to #11101. Unfortunately, this is not an option to me. :/

[patak-dev]

@iMrDJAi are these issues tracked? Have you created issues for them? Please link them here for reference if that is the case.

[iMrDJAi]

@patak-dev Related issues has been already reported (#11299). Some libraries have incorrect package.json exports, and there is no real workaround rather than reaching out to the maintainers of the affected ones to fix the problem, and this would take a ton of time just to identify what libraries having this issue. ¯\_(ツ)_/¯

[brillout]

I tried using ssrEmitAssets for vite-plugin-ssr but there are a couple of issues:

• Assets imported by isomorphic code are generated twice.
• These duplicated assets don't have the same hash and therefore have different file names (despite the content being the same).
• The files are emited in dist/server/assets/ instead of dist/client/assets/.
• The manifest now lives in two different places dist/client/manifest.json and dist/server/manifest.json.

I didn't try, but I think it's fixable by 1. moving files from dist/server/assets/ to dist/client/assets/, 2. remove duplicates by analyzing the manifest, and 3. merging the two manifests. I'll try it at some point in the future and see if I stumble upon other issues.

I think it's pretty clear at this point that we're hitting a limit regarding the fact that Vite builds the client and the server in isolation and independently of each other. (I agree with Rich-Harris and I think we can have both flexbiility while having Vite build the client and the server hand-in-hand.)

[Rich-Harris]

These duplicated assets don't have the same hash

Does this only apply to .css files? If so, it should be fixed by #5619 (comment)

[brillout]

I see, thanks for the pointer.

…

On Fri 3. Mar 2023 at 17:56, Rich Harris ***@***.***> wrote: These duplicated assets don't have the same hash Does this only apply to .css files? If so, it should be fixed by #5619 (comment) <#5619 (comment)> — Reply to this email directly, view it on GitHub <#11430 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVQRVJEOKAWFL7TLWSBEDW2IPFDANCNFSM6AAAAAATDX24YM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[eltigerchino]

Current workaround: explicitly specifying build.minify: 'esbuild' seems to fix the duplicate CSS files with differing filename hash issue for me.

[patak-dev]

This experimental feature is now used by SvelteKit, Astro, Redwood, and others. We're looking to stabilize it in Vite 5. If you have feedback before we move forward, let's use this discussion:

• Stabilizing `build.ssrEmitAssets` #13808