chore: replace ssrLoadModule with the RunnableDevEnvironment API - #16490
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/77883dc418786b14d3c0f431a42fc8a2b5cb7f7dOpen in |
|
| try { | ||
| vite.ssrFixStacktrace(error); | ||
| } catch { | ||
| // ssrFixStacktrace can fail on StackBlitz web containers and we don't know why | ||
| // by ignoring it the line numbers are wrong, but at least we can show the error | ||
| } |
There was a problem hiding this comment.
got rid of an indent, hence it looks like all the stacktrace fixing has changed.
AFAIK Vite already applies this stack trace fix when you use the environment API instead of ssrLoadModule
| remove_static_middlewares(vite.middlewares); | ||
|
|
||
| vite.middlewares.use(async (req, res) => { | ||
| // Vite throws a Cannot read properties of undefined (reading 'wrapDynamicImport') |
There was a problem hiding this comment.
This happens whenever someone runs Vitest with browser testing (you can reproduce by running pnpm test:unit in the basic test app)
…ore `manifest_data` is assigned, crashing with a TypeError if a route file is edited before the dev server serves its first HTTP request.
This commit fixes the issue reported at packages/kit/src/exports/vite/dev/index.js:416
## Bug
In `packages/kit/src/exports/vite/dev/index.js`, commit `90ecc99` ("replace ssrLoadModule") removed the eager `await update_manifest()` that previously ran during `dev()` setup, and instead runs it lazily on the first HTTP request:
```js
vite.middlewares.use(async (req, res) => {
if (!manifest_created) {
await update_manifest(); // this assigns `manifest_data`
manifest_created = true;
}
...
```
`update_manifest()` is the only thing that assigns the module-scoped `let manifest_data;` (via `({ manifest_data } = sync.create(...))`).
However, the file watchers are registered during setup — *before* the middleware installer is returned and therefore before any request is served. The `change` watcher dereferences `manifest_data`:
```js
watch('change', (file) => {
if (timeout || !/+(page|layout|server).*$/.test(file)) return;
sync.update(svelte_config, manifest_data, file, root);
});
```
and `sync.update` (`packages/kit/src/core/sync/sync.js`) immediately iterates it:
```js
export function update(config, manifest_data, file, root) {
const node_analyser = create_node_analyser(root);
for (const node of manifest_data.nodes) { ... } // TypeError if undefined
...
}
```
### Concrete trigger
1. Start the dev server.
2. Before making any HTTP request (e.g. before opening the browser), edit a `+page.svelte`, `+layout.svelte`, or `+server.js` file.
3. The `change` watcher fires, `timeout` is `null`, the regex matches, so `sync.update(svelte_config, undefined, file, root)` is called.
4. `sync.update` throws `TypeError: Cannot read properties of undefined (reading 'nodes')`.
Prior to this PR the eager `await update_manifest()` guaranteed `manifest_data` was assigned before any watcher could fire, so this window did not exist.
## Fix
Guard the `change` handler with an early return when `manifest_data` is still undefined. In that case there is nothing to incrementally update — the manifest will be created from scratch (`sync.create`) on the first request via the deferred `update_manifest()` call. This preserves the intended deferral of the `runner.import`-dependent work (which is why the eager call was removed) while avoiding the crash.
```js
watch('change', (file) => {
if (!manifest_data) return;
if (timeout || !/+(page|layout|server).*$/.test(file)) return;
sync.update(svelte_config, manifest_data, file, root);
});
```
Note the `add`/`unlink` watchers call `update_manifest` (which assigns `manifest_data` itself and wraps the `runner.import` work in a try/catch), so they are not affected by this window.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: teemingc <chewteeming01@gmail.com>
| vite.middlewares.use(async (req, res) => { | ||
| // Vite throws a Cannot read properties of undefined (reading 'wrapDynamicImport') | ||
| // if you try to run ssr.runner.import before the server has started so | ||
| // we do it inside here to avoid that |
There was a problem hiding this comment.
I think this error actually comes from @vitest/mocker's dynamic import wrapper rather than Vite itself (load_explicit_env imports fine on a server that never listens). If so, the add/unlink watchers can still crash a Vitest watch run since they call update_manifest directly.
There was a problem hiding this comment.
Hmm maybe it'll be better to move the load_and_validate out of the update_manifest so that we can do it earlier as it originally was
There was a problem hiding this comment.
Definitely, but make sure that validation still reruns on route changes
There was a problem hiding this comment.
that validation only landed in #16189, so the "original" update_manifest predates it. matchers() might be the tidy spot: fresh routes per request, and it would also catch params file edits, which currently do not retrigger validation
There was a problem hiding this comment.
hang on, update_manifest is synchronous, it returns void. so this will re-run on every request
There was a problem hiding this comment.
errr wait. i might be looking at old code
| if (module) { | ||
| server.moduleGraph.invalidateModule(module); | ||
| } | ||
| invalidate_module(server, id); |
There was a problem hiding this comment.
This used to only invalidate before the full reload, now it reloads the module in every environment too. Was that deliberate or just a side effect of sharing the helper?
There was a problem hiding this comment.
I knew about it but didn't think there would be much of an impact. The full reload takes place anyway
|
Under Vitest browser mode nothing reaches kit's middleware, so |
Co-authored-by: Nic Polumeyv <nicolas.polum@outlook.com>
Co-authored-by: Nic Polumeyv <nicolas.polum@outlook.com>
Rich-Harris
left a comment
There was a problem hiding this comment.
not totally clear on the init_manifest conversation, but it feels like something we can iterate on if necessary
|
gonna go ahead and merge this (once green) independently of the rest of the stack, in the name of reducing merge conflict risk with other PRs |
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
closes #11932
Makes sense to split this off from #16464 since it's a relatively simple change. Technically, this means we're 100% on the environment API, but everything still runs on Node.js only. Main benefit is that we have separate module graphs between the client and server now and one step closer to adopting other parts of the environment API (fetchable dev environments)
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