chore: separate generated from non-generated server code

[Rich-Harris]

This is a bit of a Hal-changing-a-lightbulb PR, but I think it's a good change anyway.

The idea was to solve #7967 by writing a second client manifest after the build has taken place. To do that we need the build analysis that happens in prerender.js, but it already felt weird to do non-prerendering stuff in that module, so I changed it to postbuild and made prerender just one of the things that happens there. But then I was irked by the fact that you need to call override before you need the Server in order to prevent the analysis from failing, so I thought perhaps we should separate those modules from each other, in the process moving the Server class out of a giant string in build_server.js and into its own module, where we get the benefits of things like type-checking.

This now means that the module exporting Server doesn't also export a weird undocumented override function, which is nice. But as yet I'm nowhere closer to fixing #7967 (to be honest I'm not yet sure how feasible it is?), and there's a bit of tidying up still to do.

Update

I saw an opportunity to reduce some duplication by using the Server class at dev time, instead of calling the lower-level respond function. This approach makes a lot more sense to me.

It does, however, mean that we have to run the src/runtime/server code through Vite, much as we already run src/runtime/client through Vite. This comes with a couple of interesting consequences:

• We need to order to ensure that instanceof HttpError and instanceof Redirect checks continue to work. But control.js, which defines these classes, can be loaded through Vite (so that our own code can reference them) and through Node (so that apps can import error and redirect from @sveltejs/kit). There's some mildly unpleasant gymnastics involved in making sure that you end up with references to the same object whether you come via Vite or Node
• Something very fucking weird happens to catch (error) {...} blocks that are transformed by Vite. I wasn't able to fully diagnose it, but changing it to catch (e) {...} miraculously fixes it

As well as de-duplicating code, this approach allows us to use if (__SVELTEKIT_DEV__) blocks instead of passing around a dev boolean — unlike before, this code will be DCE'd from production builds.

Some things I need to check:

• updating or changing src/app.html or src/error.html needs to cause a resync
• hooks should be updated when src/hooks.server.js is edited
• adding or removing src/service-worker.js should work

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. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 8aa8d12

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

[Rich-Harris]

But as yet I'm nowhere closer to fixing #7967 (to be honest I'm not yet sure how feasible it is?)

Ok I think it is possible, without running Vite multiple times. Leaving myself breadcrumbs for the morning: we basically need to split build_server into two parts — one that calls Vite, and one that generates output/server/nodes. For want of a better name call them build_server_bundle and build_server_nodes.

If we call build_server_bundle and then call build_server_nodes without client.vite_manifest, we can load the server code in order to analyse it, but we can't prerender anything (because we need client.vite_manifest to render the correct asset links).

Once we've done that, we can write a more accurate generated/nodes, with knowledge of what each entry point exports. Once we've done that, we can do the client build, which gives us client.vite_manifest.

We can then call build_server_nodes again (this part is very quick, so it's no problem to run it twice) with client.vite_manifest. Voila, we have a complete build, the client knows which routes it needs to load __data.json for, but we haven't had to run Vite more times than necessary.

[benmccann]

I believe __SVELTEKIT_DEV__ was causing issues before, so reintroducing it could be problematic. I filed a bug to investigate the issue separately: #8492

[dummdidumm]

Do you remember which kind of issues this were? I don't remember any, just that it makes testing a little more clunky.

There's also Rich's point about unit-testing dev vs prod behavior. AFAIK we didn't do that previously, and we could still do it through e2e tests, so I'm not sure how big of a deal it really would be.

Is this ready to merge? Could we merge this to unblock other changes and think about DEV vs __SVELTEKIT_DEV separately? To the user it shouldn't make a difference what we use under the hood.

[Rich-Harris]

AFAIK we didn't do that previously

we do, in e.g. csp.spec.js. but at present we have to do it by passing an option to the relevant function in order to unit test it, which sucks because it means you can't DCE the dev code in prod. Using esm-env instead of __SVELTEKIT_DEV__ — if it can be made to work — would have the same drawback.

As far as I'm concerned this is ready, though given the magnitude of the changes I'd like it to be in a release by itself so that if it introduces any subtle bugs they're easier to deal with.

[benmccann]

Stretching my memory here, but actually I think the issue wasn't with __SVELTEKIT_DEV__ itself, but import { dev } from '$app/env' and import.meta.env.SSR which we aren't using here, so hopefully the __SVELTEKIT_DEV__ thing won't be an issue. Let's not block on it

[dummdidumm]

One thing I don't quite understand yet - my last question before approving this - is why we now need to run some of the code through Vite that we ran through Node before. Does this have any other implications? Does this tie us closer than we'd like to Vite in the sense that it's no longer "just" a matter of writing a different build tool plugin to call our core code?

[Rich-Harris]

That's what allows us to use Server in dev, because it references generated code that is accessed via the __GENERATED__ alias. Without it, we have to construct the SSROptions object anew for each request, which means duplicated code and (albeit trivial) duplicated work.

Running through Vite also enables DCE (because we no longer need options.dev), which is a small win today but means we could potentially be much more ambitious with dev-time checks in future, if we wanted to be.

And it doesn't tie us any closer to Vite (unless we actively choose to start using Vite features in the server runtime) — if we were to offer a $BUILD_TOOL_X integration, its dev server would need to replicate Vite's define and alias features, but it would need those to be a viable replacement regardless.

[Rich-Harris]

added a changeset even though there's (hopefully!) no user-visible changes, so that this triggers a release

[chrskerr]

Previously in file packages/kit/src/exports/vite/dev/index.js the body and head replaces were after nonce and assets, just in case someone attempted to reference them within compiled svelte code. Should that still be the case here?

[Rich-Harris]

It makes no difference — no replacement is happening at runtime

[nstuyvesant]

Maybe a coincidence but after upgrading from @sveltejs/[email protected] to @sveltejs/[email protected], my builds starting failing on the last of these three lines:

import { env } from '$env/dynamic/private'

const { GOOGLE_RECAPTCHA_SECRET } = env
const BRAINTREE_DESCRIPTOR = JSON.parse(env.BRAINTREE_DESCRIPTOR || '')

This is what that environmental variable looks like:

BRAINTREE_DESCRIPTOR={"name": "SH Yoga*Class Workshop", "phone": "212-555-1212", "url": "something.com"}

Would this change have affected this?

[benmccann]

@nstuyvesant it's impossible to know what could possibly be wrong from just what you've shared. You'd need to file an issue with a repository to reproduce the issue and sharing the error message or stack trace you're getting