fix: skip client build if all routes have CSR disabled#15936
Conversation
🦋 Changeset detectedLatest commit: 1068cd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I don't think this is right. And it's not like we can opt out of client-side navigation in those cases, because we would need to either a) store the information about which pages have So I think the only time we would want to get rid of these files is if we know that all routes are |
Ah yeah, I overlooked that.
Skipping the client build might be a little tricky. We still use the CSS files generated from that build, which is also why we couldn’t simply omit the nodes from the client build but had to delete them after the build. |
|
Ah. We don't generate CSS files during the server build? |
|
We do but our current logic parses the client manifest to associate the client stylesheets with the route. Let me see if I can rework that to use the server stylesheets instead... |
| } | ||
|
|
||
| /** path to the `.svelte-kit` directory */ | ||
| const out_dir = normalizePath(kit.outDir); |
There was a problem hiding this comment.
Moved this out of the for loop so that it only needs to be done once.
| internal.set_read_implementation((file) => createReadableStream(`${server_root}/server/${file}`)); | ||
|
|
||
| // first, build server nodes without the client manifest so we can analyse it | ||
| build_server_nodes(out, config, manifest_data, server_manifest, null, null, null); |
There was a problem hiding this comment.
Moved this to vite/index.js so we can pass it server chunks
There was a problem hiding this comment.
Made this more generic so that it fully builds the server nodes even if it only gets the server manifest and no client manifest
| }, | ||
| experimental: { | ||
| // Allows us to use relative paths in as many places as we can | ||
| renderBuiltUrl(filename, { ssr, hostType }) { |
There was a problem hiding this comment.
We already do this in version-3 because Vite's buildApp hook doesn't accept multiple base configs. This also ensures the server and client builds emit CSS with the correct asset paths
| /** @type {Manifest | null} */ | ||
| let client_manifest = null; | ||
|
|
||
| if (!skip_client_build) { |
There was a problem hiding this comment.
All the changes below this essentially wraps the client build stuff in an if block.
| ); | ||
| } | ||
|
|
||
| secondary_build_started = false; |
There was a problem hiding this comment.
Moved this into the if block that executes the client build.
There was a problem hiding this comment.
Mostly changes to accept the manifest._.client arg rather than the entire manifest.
| expect(files).toBe(false); | ||
|
|
||
| await page.goto('/assets'); | ||
| const src = await page.locator('img').getAttribute('src'); |
There was a problem hiding this comment.
Check that assets are still public even if we skip the client build
| minify: false | ||
| minify: false, | ||
| // TODO: remove when we stop testing for vite on node 18 | ||
| assetsInlineLimit: 0 |
There was a problem hiding this comment.
?no-inline import query string doesn't work on older Vite versions which we still test for
Assets in the public directory `static` are not copied to the `output/client` directory during the server build. This means if the client build is skipped (which is now possible because of #15936), the public assets are never copied over. This PR manually copies these assets over because setting the Vite option `copyPublicDir: true` simply copies them to the `output/server` directory. --- ### Please don't delete this checklist! Before submitting the PR, please make sure you do the following: - [ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs - [x] This message body should clearly illustrate what problems it solves. - [x] Ideally, include a test that fails without this PR but passes with it. ### Tests - [ ] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check` ### Changesets - [ ] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running `pnpm changeset` and following the prompts. Changesets that add features should be `minor` and those that fix bugs should be `patch`. Please prefix changeset messages with `feat:`, `fix:`, or `chore:`. No changeset because #15936 hasn't been released yet. ### Edits - [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @sveltejs/kit@2.65.0 ### Minor Changes - feat: allow queries to refresh other queries ([#16012](#16012)) ### Patch Changes - fix: dedupe remote data ([#15991](#15991)) - fix: skip client build if all routes have CSR disabled ([#15936](#15936)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to version-3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `version-3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `version-3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @sveltejs/kit@3.0.0-next.3 ### Minor Changes - feat: allow queries to refresh other queries ([#16012](#16012)) ### Patch Changes - fix: dedupe remote data ([#15991](#15991)) - fix: skip client build if all routes have CSR disabled ([#15936](#15936)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This is probably related to #16013 which is an issue with setting |


closes #9703
Seeing if we can remove client nodes if they're unused. Disabling CSR in combination with server-side resolution should help obscure routes.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits