chore: remove generated root.svelte file#16301
Merged
Merged
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/21cf76ee078b0de627b662d007c70e9040c71f52Open in |
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
teemingc
reviewed
Jul 10, 2026
teemingc
reviewed
Jul 10, 2026
|
|
||
| data = { ...data, ...node.data }; | ||
|
|
||
| // TODO this is undefined sometimes... where does the default error component come from? |
Member
There was a problem hiding this comment.
It's undefined when SSR is disabled. Logic here
Member
Author
There was a problem hiding this comment.
ah right. so convoluted. badly needs a cleanup but that can be a follow-up
teemingc
reviewed
Jul 10, 2026
Contributor
There was a problem hiding this comment.
Additional Suggestion:
preloadData() rejects instead of resolving to the documented { type: 'error', error, status } shape when a preloaded route's load throws, because the simplify commit made load_route throw on preload errors but left preloadData's old return-based error handling in place (now dead code).
…{ type: 'error', error, status }` shape when a preloaded route's `load` throws, because the `simplify` commit made `load_route` throw on preload errors but left `preloadData`'s old return-based error handling in place (now dead code).
This commit fixes the issue reported at packages/kit/src/runtime/client/client.js:2412
## Confirmed regression
Commit `4e5dbcd` ("simplify") changed the client preload flow in `packages/kit/src/runtime/client/client.js`.
### Before
`load_route` converted preload errors into a resolved result via `preload_error(...)`:
```js
if (preload && preload_tokens.has(preload)) {
return preload_error({ error: handled_error, url, params, route });
}
```
which produced `{ type: 'loaded', state: { error }, props: { page: { ...page, status: error.status } } }`. So `_preload_data` resolved, and `preloadData` could read `result.state.error` and return `{ type: 'error', status, error }`.
### After
`load_route` now throws on preload errors (lines ~1214 and ~1304):
```js
if (preload && preload_tokens.has(preload)) {
throw handled_error;
}
// ...
if (preload && preload_tokens.has(preload)) {
throw await handle_error(err, { params, url, route: { id: route.id } });
}
```
The commit correctly updated `setup_preload` (the `data-sveltekit-preload-data` path) to use `.catch(...)`, but **forgot to update the public `preloadData` API**, which still did:
```js
const result = await _preload_data(intent); // <-- now rejects
...
if (result.type === 'loaded' && result.state.error) { // <-- dead code
return { type: 'error', status, error: result.state.error };
}
```
### Failure mode
Concrete trigger: call `preloadData('/some/route')` where that route's `load` (or `+page.server.js` load) throws. Previously it resolved to `{ type: 'error', status, error }`; now `_preload_data` rejects, so the `await` throws and `preloadData` **rejects** instead of resolving. The `if (result.state.error)` branch is now unreachable dead code.
### Contract violation
The public type declaration (`packages/kit/types/index.d.ts:~3413`) and the JSDoc still promise a resolution of shape:
```ts
Promise<({ type: 'loaded'; data } | { type: 'redirect'; location } | { type: 'error'; error: App.Error }) & { status: number }>
```
No changeset documents dropping the `{ type: 'error' }` resolution, so this is an unintended breaking change to a public API rather than a deliberate contract change.
## Fix
Wrap the `_preload_data(intent)` call in `preloadData` in a `try/catch`. On rejection, the caught value is the handled error — an `App.Error` that carries a `status` (both `handle_error`'s non-`HttpError` return `{ ...app_error, status }` and `HttpError.body` include `status`). Return it in the documented shape, with a `500` fallback for safety, and remove the now-dead `result.state.error` branch:
```js
*/ (error);
return { type: 'error', status: handled?.status ?? 500, error: handled };
}
```
This restores the documented `{ type: 'error', error, status }` resolution.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
dummdidumm
reviewed
Jul 10, 2026
|
|
||
| current_node.error = (await error_loader()).component; | ||
| current_node.component = node.node.component; | ||
| current_node.data = data; |
Member
There was a problem hiding this comment.
is this whole thing still the same fine/coarse-grained'ness as before? i.e. does each data prop update independently still? I kinda have a hard time understanding how.
Member
Author
There was a problem hiding this comment.
good catch. no, it didn't — now fixed, albeit with a bit of hackery that I want to get rid of
Member
Author
|
This isn't done, in the sense that there's a lot more tidying up that can still happen, but that tidying up is ancillary to the main point of this PR which was to get rid of |
…stor's data changes but the descendant's own `load` does not re-run, because the data-reuse `else` branch reassigns the previous render's fully-merged data.
This commit fixes the issue reported at packages/kit/src/runtime/client/client.js:845
## Bug
In `get_navigation_result_from_branch` (`packages/kit/src/runtime/client/client.js`, the `if` block now at ~line 845 after merge `4821929`), the fine-grained data-reuse optimization decides per-node whether to reuse the previous render's merged `data` object:
```js
if (
!previous_node ||
node?.data !== prev?.data ||
node.node.component !== prev.node.component
) {
current_node.data = { ...data, ...node.data };
data_changed = true;
} else {
// reuse previous object
current_node.data = previous_node.data;
}
data = current_node.data;
```
The reuse condition only inspects the node's **own** segment data reference (`node.data !== prev.data`) and its component. It ignores whether the accumulated **ancestor** `data` (built from parent layouts earlier in this linear path) changed. `previous_node.data` is the previous render's fully-merged object, which contains the OLD ancestor values.
### Concrete trigger
1. A `+layout.js` load re-runs (e.g. after `invalidate()`) and returns a new object → at the layout's index `node.data !== prev.data`, so `data` becomes the fresh layout data and `data_changed` is set.
2. A descendant `+page` that does **not** call `await parent()` and isn't otherwise invalidated: in `load_route`, `has_changed` only forces a re-run when `uses.parent && parent_changed`, so it returns the **same** previous `BranchNode` (`if (valid) return previous;`). Hence at the page's index `node.data === prev.data` and the component is unchanged.
3. The page therefore hits the `else` branch and gets `current_node.data = previous_node.data` — the OLD merged data with STALE layout values — instead of `{ ...newLayoutData, ...pageOwnData }`.
Result: the page's `data` prop and the top-level `page.data` store show outdated layout data, breaking the guarantee that a page's `data` reflects the merge of all ancestor layout data even without `await parent()`.
## Fix
Add `data_changed ||` to the front of the condition. Because the loop walks the active linear path top-down (layout → … → page) and `data_changed` is only ever set to `true`, at any iteration `data_changed === true` means an ancestor earlier in the path changed. In that case the accumulated `data` differs from the previous render, so the node must re-merge (`{ ...data, ...node.data }`) rather than reuse `previous_node.data`.
```js
if (
data_changed || // an ancestor changed → accumulated data differs → must re-merge
!previous_node ||
node?.data !== prev?.data ||
node.node.component !== prev.node.component
) {
current_node.data = { ...data, ...node.data };
data_changed = true;
} else {
current_node.data = previous_node.data;
}
```
This preserves the original fine-grained reuse (object identity is retained when nothing upstream in the path changed and the node's own data/component are unchanged), while correctly propagating updated ancestor data to descendants. The existing use of `data_changed` for `page_changed` (~line 872) is unaffected, since in the newly-covered cases it was already `true` due to the changed ancestor.
## Status
Verified against the post-merge code (commit `4821929`). The reuse condition still lacks `data_changed ||`; the bug is unaddressed. The existing patch applies cleanly (git fuzz absorbs the line shift). Line number updated from 858 → 845.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
Rich-Harris
commented
Jul 12, 2026
Comment on lines
+18
to
+19
| <a href="/state/data/state-update/a">a</a> | ||
| <a href="/state/data/state-update/b">b</a> |
Member
Author
There was a problem hiding this comment.
drive-by fix (the test was using goto, so these bad links didn't cause a CI failure)
teemingc
reviewed
Jul 12, 2026
| "@opentelemetry/api": "^1.0.0", | ||
| "@sveltejs/vite-plugin-svelte": "^7.0.0", | ||
| "svelte": "^5.48.0", | ||
| "svelte": "^5.56.4", |
Member
There was a problem hiding this comment.
We'll need to adjust the changeset note in .changeset/heavy-showers-leave.md too
teemingc
approved these changes
Jul 12, 2026
teemingc
left a comment
teemingc
left a comment
Member
There was a problem hiding this comment.
LGTM just need to edit the changeset about the peer Svelte version
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
been looking forward to this for so long — getting rid of the generated
root.sveltecomponent and replacing it with a real one. we're only now in a position to make this change. some type stuff that needs fixing still but i think this is most of the way there. (I do also want to switch to usingmountinstead of the legacy class interface)we can probably simplify some other stuff as a consequence of this, eventually
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