Skip to content

fix: skip client build if all routes have CSR disabled#15936

Merged
teemingc merged 24 commits into
mainfrom
remove-unused-client-nodes
Jun 8, 2026
Merged

fix: skip client build if all routes have CSR disabled#15936
teemingc merged 24 commits into
mainfrom
remove-unused-client-nodes

Conversation

@teemingc

@teemingc teemingc commented May 31, 2026

Copy link
Copy Markdown
Member

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:

  • 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
  • This message body should clearly illustrate what problems it solves.
  • 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:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@changeset-bot

changeset-bot Bot commented May 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1068cd7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@svelte-docs-bot

Copy link
Copy Markdown

@teemingc teemingc marked this pull request as ready for review June 1, 2026 14:27
@Rich-Harris

Copy link
Copy Markdown
Member

I don't think this is right. csr = false means that we don't load the client if we land on the page directly. If we navigate to the page from a different page with csr = true, then we can't 'unload' the client, we just treat it as a normal client-side navigation. With this change, that's impossible, because the client will attempt to import a non-existent module.

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 csr = false in the route manifest or b) fall back to a hard reload after a soft navigation failed. Neither seems desirable.

So I think the only time we would want to get rid of these files is if we know that all routes are csr = false, and in that case we'd just skip the client build altogether.

@teemingc

teemingc commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

I don't think this is right. csr = false means that we don't load the client if we land on the page directly. If we navigate to the page from a different page with csr = true, then we can't 'unload' the client, we just treat it as a normal client-side navigation. With this change, that's impossible, because the client will attempt to import a non-existent module.

Ah yeah, I overlooked that.

So I think the only time we would want to get rid of these files is if we know that all routes are csr = false, and in that case we'd just skip the client build altogether.

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.

@Rich-Harris

Copy link
Copy Markdown
Member

Ah. We don't generate CSS files during the server build?

@teemingc

teemingc commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

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...

@teemingc teemingc marked this pull request as draft June 2, 2026 17:35
@teemingc teemingc changed the title fix: remove unused client nodes if CSR is disabled fix: skip client build if all nodes have CSR disabled Jun 8, 2026
@teemingc teemingc changed the title fix: skip client build if all nodes have CSR disabled fix: skip client build if all routes have CSR disabled Jun 8, 2026
}

/** path to the `.svelte-kit` directory */
const out_dir = normalizePath(kit.outDir);

@teemingc teemingc Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved this out of the for loop so that it only needs to be done once.

@teemingc teemingc marked this pull request as ready for review June 8, 2026 11:57
Comment thread packages/kit/src/exports/vite/index.js Outdated
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);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved this to vite/index.js so we can pass it server chunks

@teemingc teemingc Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 }) {

@teemingc teemingc Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All the changes below this essentially wraps the client build stuff in an if block.

);
}

secondary_build_started = false;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved this into the if block that executes the client build.

@teemingc teemingc Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

?no-inline import query string doesn't work on older Vite versions which we still test for

@Rich-Harris Rich-Harris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

outstanding!

@teemingc teemingc merged commit 3968fb9 into main Jun 8, 2026
29 checks passed
@teemingc teemingc deleted the remove-unused-client-nodes branch June 8, 2026 22:39
This was referenced Jun 8, 2026
teemingc added a commit that referenced this pull request Jun 10, 2026
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.
Rich-Harris pushed a commit that referenced this pull request Jun 11, 2026
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>
Rich-Harris pushed a commit that referenced this pull request Jun 11, 2026
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>
@charbelnicolas

Copy link
Copy Markdown
image_2026_06_11_22_03_28

I'm getting 404 errors all over the place after updating sveltekit and this seems to be the culprit. What am I supposed to do?

@teemingc

teemingc commented Jun 12, 2026

Copy link
Copy Markdown
Member Author
image_2026_06_11_22_03_28 I'm getting 404 errors all over the place after updating sveltekit and this seems to be the culprit. What am I supposed to do?

This is probably related to #16013 which is an issue with setting paths.relative: false in the kit config.

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.

adapter-static generates unused js chunks when csr=false

3 participants