fix: load env before prerender

[paoloricciuti]

If you access dynamic env in a remote file (even through other meanings like importing the default generated db when you do sv create) the value will be empty during the prerendering of the remote functions...even worse if you have a situation like this (like the default generated db when you do sv create)

import { query } from '$app/server';
import { env } from '$env/dynamic/private';

if (!env.SOME_VAR) throw new Error('SOME_VAR is not set');

export const get = query(async () => {});

it will throw even if SOME_VAR is set because in the prerender function we don't set the environment line in analyse

This PR fixes that. I would appreciate some pointer on where to write such test (I know we shouldn't add app like if they were free :D )

---

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 detected

Latest commit: b2d1de4

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]

Isn't this somewhat by design? If you access env.* when prerendering a page normally, you will get an error:

Cannot read values from $env/dynamic/private while prerendering (attempted to read env.MESSAGE). Use $env/static/private instead

We mention it in the docs:

Dynamic environment variables cannot be used during prerendering.

Feels like this would be a way to circumvent that restriction, which is there for a good reason. Can we error when accessing dynamic env vars in remote files during prerendering, same as we do otherwise?

[paoloricciuti]

Uh i wasn't aware of this restriction...the problem is that since we are importing the remote files during prerendering even if there's no prerender invocation (because we need to check if there's one) this will prevent remote functions to import any module that uses dynamic env (which again is the db by default when we create with sv create).

We should at least throw a more descriptive error but it also feels like kinda of an heavy restriction considering remotes are almost designed to access env (and while you can always use static (which I like more tbh) sometimes you might need dynamic.

[Rich-Harris]

Ideally you wouldn't be initializing your db using dynamic env vars during analysis/prerendering — that would be gated on !building, if it blows up otherwise

[mmiddlezong]

Maybe a change should be aimed at the sv cli instead (specifically drizzle)?

[vdemcak]

What about wrapping the drizzle init code in a function and then calling it from the init method in hooks.server.ts? Something like this:

// src/hooks.server.ts

import { createPostgresClient } from '$lib/server/database';
import type { ServerInit } from '@sveltejs/kit';

export const init: ServerInit = async () => {
    await initDatabase();
};

// src/lib/server/database/index.ts

export let client;
export let db;

export function initDatabase() {
	if (!env.DATABASE_URL) throw new Error('DATABASE_URL is not set');

	client = neon(env.DATABASE_URL);
	db = drizzle(client, { schema });
}

[paoloricciuti]

Yeah I think we would also need to change that...but that's just generated code, you could write similar code yourself and it wouldn't feel weird to write.

[Rich-Harris]

Thinking this through... the original prohibition on using dynamic env vars while prerendering was a response to #10008. There were two issues:

1. prerendered pages would include build-time variables instead of runtime ones, and a) they could differ and b) it theoretically risks exposing information that wasn't meant to be exposed
2. if you landed on a prerendered page, the content of $env/dynamic/public would be populated with build-time variables, which would persist if you then navigated to a dynamically rendered page

The first of these feels like a bit of a shrug to be honest. If you're prerendering then you're choosing to use build-time variables, and I'm not exactly sure what the risks of exposure are — if you render JSON.stringify(env) then sure you might expose some build-time variables but... don't do that?

The second is a real thing. But it was fixed by populating public env from a generated endpoint in the case where the user lands on a prerendered page, instead of embedding vars in the HTML. That would continue to be the case if we allowed dynamic env var access when analysing/prerendering remote functions.

So... maybe I take back what I said above and this is fine?

[teemingc]

The presence of this file alone is the test for if this works or not, right? Can we add a comment to this file explaining that if that’s the case?

[Rich-Harris]

https://github.com/sveltejs/kit/pull/14219/files#r2278077682

[paoloricciuti]

The second is a real thing. But it was fixed by populating public env from a generated endpoint in the case where the user lands on a prerendered page, instead of embedding vars in the HTML. That would continue to be the case if we allowed dynamic env var access when analysing/prerendering remote functions.

Does setting the envs at the beginning of the prerender phase breaks this? I think the prerender function is also used to prerender prerendered pages no? Also would this break something or should we guard the code I added with if(experimental.remoteFunctions)

[Rich-Harris]

Does setting the envs at the beginning of the prerender phase breaks this?

Been digging into this and... not exactly. The error happens because of this — inside server.init during prerendering, env is set to a Proxy that errors if you try to access anything:

kit/packages/kit/src/runtime/server/index.js

Lines 55 to 60 in d89d6e2

	set_private_env(
	prerendering ? new Proxy({ type: 'private' }, prerender_env_handler) : private_env
	);
	set_public_env(
	prerendering ? new Proxy({ type: 'public' }, prerender_env_handler) : public_env
	);

We don't call server.init until here...

kit/packages/kit/src/core/postbuild/prerender.js

Lines 497 to 501 in d89d6e2

	const server = new Server(manifest);
	await server.init({
	env,
	read: (file) => createReadableStream(`${config.outDir}/output/server/${file}`)
	});

...which is later than the internal.set_<type>_env(env) calls in this PR, and — crucially — later than the bit where we eagerly load all the remote modules:

kit/packages/kit/src/core/postbuild/prerender.js

Lines 480 to 489 in d89d6e2

	for (const loader of Object.values(manifest._.remotes)) {
	const module = await loader();
	for (const fn of Object.values(module)) {
	if (fn?.__?.type === 'prerender') {
	prerender_functions.push(fn.__);
	should_prerender = true;
	}
	}
	}

The end result is that if you have env.BLAH at the top of a .remote.ts file, it will work, but if you reference env.BLAH while prerendering a page or an endpoint or a remote function, it will error.

This troubles me a bit, because it means that these equivalent bits of code will behave very differently:

let message = env.MESSAGE;

export const get_message = prerender(() => {
  return message;
});

export const get_message = prerender(() => {
  return env.MESSAGE;
});

It also means that some top-level occurrences will be fine, while others won't be, depending on which modules happened to be imported before the server.init call (as opposed to during routing, for example).

We really should have consistency here. And while we could presumably achieve that by putting the server.init call earlier, such that all env.* references cause an error, that will make life unnecessarily hard for people.

So I'm forced to conclude that we should just allow env.* references during prerendering. This does mean that in a case like this...

<script>
  import { env } from '$env/dynamic/public';
</script>

<h1>{env.PUBLIC_MESSAGE}</h1>

...the contents of the <h1> could change upon hydration, if the page was prerendered with a different set of env vars. But the solution is 'don't do that'. Does that seem like a reasonable outcome? (The important thing is that landing on a prerendered page won't 'poison' $env/dynamic/public for all subsequent dynamically rendered pages — this will continue to be true.)

Another thing I noticed while looking into this — turns out we don't abort if a prerender remote function fails, we just save the error to disk. That's not helpful. We probably want an equivalent of the prerender.handleHttpError function, or to use the same handler with new options. Opened #14242

[Rich-Harris]

Opened #14243

[vdemcak]

Can't the pre-renderer just throw an error or skip pre-rendering of content that uses dynamic env?

[Rich-Harris]

How?

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-kit-14219-svelte.vercel.app/