Consistent handling of environment variables

Rich-Harris commented 2 years ago

Describe the problem

SvelteKit doesn't do anything special with environment variables; it delegates to Vite. This is good for the sorts of environment variables Vite deals in — VITE_ prefixed values that are replaced at build time — but less good for other kinds.

Environment variables can be static (known at build time) or dynamic (not known until run time), and private (only accessible to code that runs on the server) or public (accessible to code that runs on the server or the client):

	static	dynamic
public	✅ import.meta.env.VITE_*	❌ (1)
private	⚠️ can be done with plugins (2)	⚠️ process.env.* (3)

To be fair, I'm not sure what sorts of things would go in this quadrant, though I would be eager to hear examples
You can add something like plugin-replace to selectively replace things at build time, but this is ad hoc, undiscoverable, and involves duplication between .env files and config
process.env is only available in Node-based environments (e.g. adapter-node, adapter-netlify and adapter-vercel — though once the Vercel adapter starts building Edge Functions that will no longer be the case)

It would be great if we had a consistent and idiomatic way of dealing with all of these.

Describe the proposed solution

I don't actually have any proposals to make, I just didn't want https://github.com/sveltejs/kit/pull/4293#issuecomment-1065300764 to get lost. Whatever solution we land on should

work with .env/.env.* files
enable dead code elimination etc at build time
allow autocompletion/auto-imports (e.g. we could generate typings from your .env file)
feel consistent across the four quadrants, but with clearly understandable distinctions between them (e.g. $app/env/public vs $app/env/private or whatever)

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

bluwy commented 2 years ago

For dynamic-public env vars, I had a usecase for it and lightly touched on it before. But the gist is that a frontend would connect to a backend URL, which can be public, but the backend URL could change for e.g. testing purposes, or it's not known in build time. But that said, it should be a userland concern, and isn't something SvelteKit/Vite has to handle IMO.

tsongas commented 2 years ago

I like https://github.com/kerimdzhanov/dotenv-flow.

bfanger commented 2 years ago

The current recommendation (according to the FAQ) is to use a getSession hook

But using import { session } from "$app/stores"; get(session).MY_VAR outside of a svelte component doesn't work, which limits its use. (It uses getContext in the subscribe, this is by-design, and desired because on the server you really want to isolate these stores per request)

Idea, part 1 (AdapterEnv)

import { env } from "$app/adapter";

console.log(env.TRACKING_ID);

The values inside the env object will be provided by the adapter, in case of adapter-node and the dev server it will be the process.env after dotenv, on adapter-deno it will be Deno.env.toObject()

The values can be used to store secrets such as connections string and are therefor only are available on the server. This import is not available on the client.

The "$app/adapter" import would also allow adapters to expose emulation for features in development like Cloudflare KV, but that's another topic

Idea, part 2 (env Hook)

To expose variables to client code we could add a getPublicEnv hook.

in hooks.js:

export function getPublicEnv(env: AdapterEnv) {
  return {
    API_ENDPOINT: env.API_ENDPOINT,
    TRACKING_ID: env.TRACKING_ID,
  };
}

The getPublicEnv hook is executed only once at startup, after that the values are set and can be imported from anywhere. (no context needed)

Idea, part 3 (generated exports)

To access the public environment variables you'd write:

import { API_ENDPOINT } from "$app/env";

Using $app/env here allows the server to also access the values from the getPublicEnv hook. The getPublicEnv must therefore return an object with the same keys in all environments.

Summary

Work with .env/.env.* files - ✅ Possible but is up to the adapter to implement
Enable dead code elimination etc at build time - ✅ Still possible by using the VITE_ variables
Allow autocompletion/auto-imports ✅ generated app/env based on the output of the hook.
Feel consistent across the four quadrants ❌ Adding getServerEnv, getPublicBuildEnv and getServerBuildEnv hooks might make the api more uniform, but doesn't add much clarity imho.

Rich-Harris commented 2 years ago

Another dimension along which we're currently inconsistent: #3040. Vite loads .env files in dev and build, but using the example .env file...

DB_PASSWORD=foobar
VITE_SOME_KEY=123

...VITE_SOME_KEY is replaced in both cases, but DB_PASSWORD only has meaning in dev. Part of me thinks this is all a bit confusing and we'd ideally bypass Vite's automatic .env loading, but that might annoy people.

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris, I'm hugely passionate about this one. Maybe it's because I was talking about it and frustrated by it even back in June of last year... I encourage you to scroll through that conversation for context about how confusing this is to someone new to the ecosystem -- the fact that Vite statically replaces process.env.foo but NOT process.env['foo'] is not straightforward. I wasted several days battling with how to get Vite to play nicely with my environment variables, and overall, the number of kludgy workarounds I have to use is just too high. Kit needs to address this, like you said, completely and consistently. Thanks for opening this issue for discussion.

Some notes, in no particular order:

I'm a TypeScript-first user, so generating types based on the .env file sounds neat, but I'd much rather be able to declare my expected environment variable shapes similarly to how I declare my Session type. Managing .env files across teams/users is tough, because they're not usually committed to the repository -- meaning their shape is documented in the README or not at all. I'd much rather have them documented through code.

As far as API goes, if I could have my way, it'd look like this:

import { env } from '$app/buildtime';
import { privateEnv } from '$app/buildtime';
import { env } from '$app/runtime';
import { privateEnv } from '$app/runtime';

Whatever solution we build should be able to guarantee that no properties on the privateEnv object can be built into client-exposed code or exposed to the client at runtime. I think in the case of environment variables, we should not reinvent the wheel. Let userland rename the variables if they want (import { env as buildtimePublicEnv } from '$app/buildtime';), and make the object returned a plain old JavaScript object. As far as the typing stuff, the default would be (just spitballing):

declare namespace App {
  interface Environment {
    buildtime: {
      env: Record<string, string>,
      privateEnv: Record<string, string>,
    }
    runtime: {
      env: Record<string, string>,
      privateEnv: Record<string, string>,
    }
  }

As a bonus, future feature, we may be able to have Kit validate that the expected keys appear in .env. If they're buildtime variables, it could throw during build if any are missing. If they're runtime variables, it could throw on initialization.

This isn't meant to be a complete proposal, per se, but more just to try to shuffle the conversation in a direction I think might be beneficial. Heck, it'd certainly be beneficial to me.

Once the team can decide on a full design spec, I'd be happy to help implement this.

luisgabrielm commented 2 years ago

Whatever solution we build should be able to guarantee that no properties on the privateEnv object can be built into client-exposed code or exposed to the client at runtime

@tcc-sejohnson I strongly agree with being clear about which variables are being exposed in the client.

I started coding in the backend, so the security concerns were rarely about mistakenly exposing a private-key to the client. And, whenever I worked with the frontend I was careful to always use a proxy API or something similar.

Since sveltekit has both client side and server side code it does make me very worried that I’ll expose something by accident. If it’s super clear what’s being used where I think that’s a major feature of the framework.

elliott-with-the-longest-name-on-github commented 2 years ago

Hey @Rich-Harris,

Like I said, I'm pretty passionate about this one -- so here's a full design proposal to get people talking.

To restate the points Rich laid out in the original comment, a solution needs to:

work with .env/.env.* files
enable dead code elimination etc at build time
allow autocompletion/auto-imports (e.g. we could generate typings from your .env file)
feel consistent across the four quadrants, but with clearly understandable distinctions between them (e.g. $app/env/public vs $app/env/private or whatever)

After thinking about it for some time, I think it should also:

be composable to some degree
guarantee from a Kit perspective that private variables are not exposed to the client
provide the ability to validate that the environment config is valid at startup/buildtime
provide a relatively easy-to-follow execution path

Let's talk about those.

Be composable to some degree

The immediate use-case I can think of is when my app needs to know an environment-dependent URL for an api:

local: http://localhost:7071
dev: https://asdfhjkl.azurewebsites.net
staging: https://staging.api.mywebsite.com
prod: https://api.mywebsite.com

Whenever I need to reference this URL within my code, I often need to change behavior based on whether I'm operating in HTTP/HTTPS. Sometimes I need to know the port. Sometimes I just need the host, etc. Sure, I can just new URL(process.env['API_URL']), but that's annoying -- I have to do it every time I need the variable. I'd much rather structure my environment config as:

API_PROTOCOL=http
API_HOST=localhost
API_PORT=7071
API_URL={API_PROTOCOL}://{API_HOST}{API_PORT ? ':' + API_PORT : ''}

There are other instances where I'd like composability (or just the ability to compose a JavaScript object out of a string config value without having to do so at every single line that uses the config value). More on this in the design section. 😺

Guarantee no client exposure of private variables

Pretty simple. Obviously, Kit can't guarantee you don't just create a GET endpoint that returns MY_PRIVATE_VARIABLE to the frontend, but it should be able to assert that accessing a private variable from the client is invalid.

Provide the ability to validate config at startup/buildtime

I've wasted more time than I probably should by forgetting to include a new environment variable in .env and then having to track down why my credentialed API call isn't working. ☹️ It'd be nice to be able to assertively validate that the environment is what is expected at buildtime and runtime.

Provide a relatively easy-to-follow execution path

There are a lot of complicated ways to implement something like this. Let's not do one of them.

Design

I propose the addition of a few new exports from the $app route, one new root file, startup.{js|ts}, one new argument to the CLI, environment, and four new types to the App namespace. Users can define environment variables through a .env | .env.{environment} file or through environment variables present on the system at buildtime (or at runtime, if supported by whichever adapter). Environment variables will override .env variables. At both startup and buildtime, the variables will be resolved to a JavaScript Record<string, string> and passed to startup.{js|ts}, where users can validate and compose their env (more on this later). From there, environment variables will be available as import { env, privateEnv } from '$app/{buildtime|runtime}'.

With that summary, let's follow the "execution flow" and describe in more detail how this would work.

`.env | .env.environment`

Same design as everyone is used to for .env files. KEY=value, supports strings. .env.{environment} is loaded when svelte-kit {do-something} --environment={environment}. (Technically, we could reuse the Vite mode here, but given that we're building our own environment config, I would think we would want to separate it conceptually from Vite as much as possible.)

In order to support our four quadrants, we'll need some sort of Vite-like prefixing scheme. I'm not particularly attached to any one scheme, so I'm open for suggestions, but for the sake of this proposal, we'll say variables prefixed with are :

BLDPRV, buildtime private
BLDPUB, buildtime public
RUNPRV, runtime private
RUNPUB, runtime public

We could also potentially allow simply PRV or PUB to indicate that the variable should be serialized during buildtime but also be available during runtime. The more specific RUNPRV/BLDPRV would take precedence.

`startup.{js|ts}`

Why isn't this file named env.{js|ts}? Well, because, we can set ourselves up to kill two birds with one stone here. (Or we can just kill one bird and call it .env.{js|ts}. Up for debate.)

This file exports four methods with the following defaults:

export const resolvePublicRuntimeEnv = (publicEnv: Record<string, string>): App.PublicRuntimeEnv => publicEnv;
export const resolvePrivateRuntimeEnv = (privateEnv: Record<string, string>): App.PrivateRuntimeEnv => privateEnv;
export const resolvePublicBuildtimeEnv = (publicEnv: Record<string, string>): App.PublicBuildtimeEnv => publicEnv;
export const resolvePrivateBuildtimeEnv = (privateEnv: Record<string, string>): App.PrivateBuildtimeEnv => privateEnv;

Each of these receives the resolved variables from .env and the environment. Users can validate the values, compose them as they wish, and return the resolved environment.

New `App` types

You'll notice I referenced types from the App namespace above. I propose the following be added:

declare namespace App {
  interface PublicRuntimeEnv extends Record<string, string>;

  interface PrivateRuntimeEnv extends Record<string, string>;

  interface PublicBuildtimeEnv extends Record<string, string>;

  interface PrivateBuildtimeEnv extends Record<string, string>;
}

Like the other App types, these can be set by the user.

Edit: The types should default to Record<string, string>, but users should be able to set them to anything extending Record<string, any> (and maybe we should allow numbers as indexers as well?).

User API

The objects returned from resolveXxxXxxEnv will be available to the developer throughout the app via import { env, privateEnv } from '$app/{buildtime|runtime}'. Importing privateEnv client-side (regardless of whether any of its properties are accessed) will result in an exception.

A full example, using my previous example

So, using the previous example:

// .env
BLDPUB_API_PROTOCOL=http
BLDPUB_API_HOST=localhost
BLDPUB_API_PORT=7071

// app.d.ts
declare namespace App {
  interface PublicBuildtimeEnv {
    apiUrl: URL;
  }
}

// startup.ts
const throwError = (missingKey: string) => throw Error(`Could not find ${missingKey} in .env or environment variables.`);

export const resolvePublicBuildtimeEnv = (env) => {
  if (!env.BLDPUB_API_PROTOCOL) {
    throwError('BLDPUB_API_PROTOCOL');
  }
  if (!env.BLDPUB_API_HOST) {
    throwError('BLDPUB_API_HOST');
  }

  const port = env.BLDPUB_API_PORT ? `:${env.BLDPUB_API_PORT}` : '';
  const apiUrl = new URL(`${env.BLDPUB_API_PROTOCOL}://${env.BLDPUB_API_HOST}${port}`);
  return { apiUrl }
}

// anywhere else in my code
<script lang="ts">
  import { env } from '$app/buildtime`
</script>

// typing provided by app.d.ts!
// 'http://localhost:7071' serialized into code at buildtime
<p>{env.url}</p>

Does this meet the requirements?

work with .env/.env.* files
- Yes
enable dead code elimination etc at build time
- Maybe? I'm not super familiar with how this works.
allow autocompletion/auto-imports (e.g. we could generate typings from your .env file)
- Absolutely.
feel consistent across the four quadrants, but with clearly understandable distinctions between them (e.g. $app/env/public vs $app/env/private or whatever)
- I think so, but up to the community to decide.

(My added requirements)

be composable to some degree
- Yes, supports resolving arbitrary environment config from Record<string, string>
guarantee from a Kit perspective that private variables are not exposed to the client
- Yes
provide the ability to validate that the environment config is valid at startup/buildtime
- Yes
provide a relatively easy-to-follow execution path
- To me, the .env => startup.ts => import { env, privateEnv } from '$app/{buildtime|runtime}' pipeline is pretty darn simple.

Quick note about `startup.{js|ts}`

Way back when, I opened #1753 to document "startup" behavior in hooks.js. Essentially, if you've got server-side startup code, running it in the root of hooks.js and waiting for it to finish in handle before serving any requests is the only way to do it.

To me, this behavior feels really bad. hooks.js doesn't feel like the place it should be done. The name isn't right, all of the other things hooks have to do have nothing to do with startup, etc. Also, having to await startup() in handle is nothing but dead code after startup is complete, but it still has to be run for every incoming server request. Ew. Introducing this startup.js file would naturally lead to a fifth export, startup, which would receive the resolved config and run (blocking) before the "server starts" (the exact behavior would likely be adapter-specific?). I'm just taking the opportunity to introduce that idea and scaffold it while we're looking at this.

Please rip this apart, provide your controversial opinions, and hate on my design. If you notice any typos or anything that seems to stupid to be an actual suggestion, let me know so that I can correct it.

😺

shiftlabs1 commented 2 years ago

@Rich-Harris can we borrrow a leaf here https://docs.astro.build/en/guides/environment-variables/ instead of letting this drag for so long . import.meta.env.WHATEVER works on Server and client side in Astro. BUT only environment variables prefixed with PUBLIC_ are available client side .note they also available server side so that code running on client and server have the same mode of extracting environment variables. I am porting a small app to astro edge functions given how long this is taking to get stamped

elliott-with-the-longest-name-on-github commented 2 years ago

@shiftlabs1

Unfortunately this would only solve a very small part of the problem. Vite variables are only available at build time (not runtime), so while this may work for static sites, if the app needs any runtime variables, it would still have to try to use some messy version of process.env or similar, which is part of what this discussion is trying to solve -- we don't want users to have to pull some environment variables from someplace (import.meta.env) and others from some other place (process.env et al).

Just a quick note about switching to Astro: Remember that SvelteKit is still beta software and the maintainers are being careful to balance getting to 1.0 with making good, long-term decisions about how to implement features. If you need a production-ready site right now, you should definitely use a production-ready software, as SvelteKit will experience breaking changes without warning, probably multiple times before 1.0.

shiftlabs1 commented 2 years ago

@tcc-sejohnson but do we even have the above to give unified access at Build time? If one can read environment variables from .env files in a unified manner without the concern at least of whether a module will run on just the server or server and client WRT environment variables, that will be helpful to most folks. and the conversation /suggestions so far can focus on runtime variables. All the suggestions so far speak more to dealing with runtime variables and not the static variables read from .env files. It will not only solve a small part of the problem as you said but a significant bit. There are more build time variables than run time in any application. If this were in place , i can deal with runtime variables with process.env i know at least that it is a runtime entry

elliott-with-the-longest-name-on-github commented 2 years ago

@shiftlabs1

Keep in mind, I'm not a maintainer, just a humble contributor working where I can help. But look at it this way: Kit is opinionated and, where possible, it likes to provide a complete, unified solution to a problem. If we were to implement a partial solution, it could actually get in the way of a complete solution later, requiring a rewrite (and associated breaking changes). Given that we're pre-1.0 and there are no time constraints around releasing features, I'd strongly vote in favor of finding a complete solution to the problem rather than hipfiring a half-baked solution just to suit a specific use case. 🤷

quasarchimaere commented 2 years ago

i do also find it rather difficult to make "runtime" environment variables work.

tl;dr: In Sapper we used a middleware in server.js that injects serverside variables into the client session (e.g. a GRAPHQL_URI). In sveltekit we use the getSession function in the hooks.js files to do the same (afaik no specific server.js is needed anymore).

This works fine in DEV mode, however in order for the production deployment to work, we have to use all used environment variables already during the build time since vite replaces them with static strings -> regardless of the way we access them in the hooks.js (process.env.GRAPHQL_URI, process.env["GRAPHQL_URI"], process["env"]["GRAPHQL_URI"] neither of them work)

What i am trying to achieve:

Build a docker image that contains the sveltekit app, and deploy it to a kubernetes cluster which injects the proper environment variables

https://discord.com/channels/457912077277855764/988390637072162876/988397666016833596

elliott-with-the-longest-name-on-github commented 2 years ago

@quasarchimaere

process.env["SOME_VAR"] should work just fine for runtime environment variables (I'm using it right now), but you have to make sure those environment variables are actually set when your run your start command. My start command is currently env_cmd -f .env.private node build. Vite only replaces the literal string proces.env., so if you're using the bracket syntax, it should not statically replace it.

This process is confusing enough that I'd be in favor of finding a way to block Vite environment variables entirely once we've implemented our solution -- from a JavaScript perspective, process.env.SOME_VAR should be the same as process.env["SOME_VAR"] and Vite confusingly corrupts that concept.

quasarchimaere commented 2 years ago

@tcc-sejohnson thank you so much for the confirmation that it should actually work. in fact i do not even need to use any .env file at all, running the node build with set environment variables works just fine without it too. the issue i ran into was a different one actually that masked itself as a process.env issue -> idk if its already one but calling the basepath of the node-build does not call getSession i have to somehow go to a subpath in order for the getSession function to inject the parameters

quasarchimaere commented 2 years ago

@tcc-sejohnson thank you so much for the confirmation that it should actually work. in fact i do not even need to use any .env file at all, running the node build with set environment variables works just fine without it too. the issue i ran into was a different one actually that masked itself as a process.env issue -> idk if its already one but calling the basepath of the node-build does not call getSession i have to somehow go to a subpath in order for the getSession function to inject the parameters

i retract that statement, this happened because i was having an prerender true export in my index.svelte start page, which obviously doesn't need to call a hook

inta commented 2 years ago

Just a little note to everyone confused by the fact that dotenv is not working within svelte.config.js. Maybe that should be obvious, but it did take some time for me to discover that I had to load dotenv inside the config myself. Just adding import 'dotenv/config' is enough and you can access the vars defined inside .env files using process.env.VITE_<whatever>.

Edit: Ok, that was a bad idea (above), because it will destroy Vites logic used to load .env files based on the "mode". I think "mode" is equal to NODE_ENV, at least that worked for me. So now I'm using import {loadEnv} from 'vite' and call loadEnv(process.env.NODE_ENV, process.cwd(), envPrefix) to load the environment variables and it works like expected.

benmccann commented 2 years ago

There are three axes along which env variables vary: mode (dev/prod/etc.), visibility (client/server), build vs runtime mode - .env.[mode] visibility - VITE_ prefix build vs runtime - import.meta.env vs process.env

We should review these to see which combinations work or not. I think there may be some things in Vite that need to be fixed like https://github.com/vitejs/vite/issues/6626

Rich-Harris commented 2 years ago

We've been talking about this in the maintainers' chat and I have a proposal that I think covers all the bases we care about.

First off, I don't think we need to care about .env files. At dev/preview time, Vite loads that stuff for us; in production you generally won't have an .env file (you might if you're managing a Node server yourself, but in many cases your env vars will be configured through a dashboard), so I don't think this falls under SvelteKit's remit. All it needs to do is expose existing environment variables sensibly.

A corollary is that we don't need to care about the mode axis. We only care about two axes — private/public and static/dynamic.

Vite covers the public/static quadrant, but does so in a way that is frankly a bit clunky. import.meta.env.VITE_FOO doesn't communicate that VITE_FOO is public, it's a lot to type out, we don't get autocompletion (I'm sure there's a plugin for it, but still), and moreover import.meta is just an awkward place to store arbitrary global stuff. (import.meta is convenient for toolmakers since it's easy to statically analyse, but it has nothing to do with importing, and as such I view it as hostile to beginners.)

My proposal:

// env vars set at build time and statically replaced
import { PRIVATE_TOKEN } from '$app/env/private';
import { PUBLIC_BASE_URL } from '$app/env';

export function GET({ env }) {
  return fetch(`${PUBLIC_BASE_URL}/stuff?theme=${env.THEME}`, {
    headers: { 
      authorization: `Bearer ${PRIVATE_TOKEN}`
    }
  });
}

`$app/env/private`

This is a generated module (with generated typings) that contain a dump of process.env at build time:

if (!import.meta.env.SSR) {
  throw new Error('Cannot import $app/env/private into client-side code');
}

export const HOME = '/Users/rich';
export const SHELL = '/bin/zsh';
export const PRIVATE_TOKEN = 'xyz123';
// ...

Unused variables are treeshaken away. Rollup is smart enough to do constant inline across module boundaries, so this gives us the same benefits (in terms of dead code elimination etc) as import.meta.env. VSCode would give us autocompletion hints (thanks to the generated types), so as soon as you started typing PRIVATE_T it would offer to import and auto-complete PRIVATE_TOKEN.

`$app/env`

This is another generated module that contains all the keys of process.env that begin with PUBLIC_:

export const PUBLIC_BASE_URL = 'http://example.com';
// ...

It can be imported wherever.

`event.env`

We can't make dynamic variables (i.e. where you have one build but multiple deployments, e.g. white-labelling scenarios) available wherever, because on some platforms they're only exposed along with a request. For example the Cloudflare Workers signature is this:

export default {
  fetch(request, env, context) {
    const theme = env.THEME;
    // ...
  },
};

For that reason, I propose making dynamic variables part of RequestEvent. People using Node-based platforms would be free to do process.env.THEME etc, but the idiomatic way would be to use event.env.

There isn't a way in this proposal to automatically expose dynamic variables to the client, but I'd argue getSession is adequate.

bfanger commented 2 years ago

Nice to see the adapter standardization for runtime environment variables and the autocompletion improvements for the build-time environments.

We write apps using the twelve-factor app methodology, therefore we create one build that deploys to all environments, so we also won't be able to use the cool autocompletion features :(

Too bad that runtime is made difficult due to Cloudflare's env per request setup. We don't use Cloudflare, so we'll stick to our handle workaround as getSession is clumsy when trying to use environment variables outside components.

Rich-Harris commented 2 years ago

Hmm. I'd imagined that event.env.FOO would be a reasonable compromise on the basis that most apps wouldn't make heavy use of runtime environment variables, but... yeah.

I wonder if we could solve this another way. Since the built SvelteKit server doesn't import any of your code until the adapter calls server.respond, perhaps we could have something like this...

// inside adapter code (SvelteKit users would never see this unless they were building an adapter)
const server = new Server(manifest);

const worker = {
  async fetch(req, env, context) {
    server.init({ env }); // calling this a second time is a noop

    // ...
    return server.respond(...);
  }
};

...which would make env available as a regular module:

// src/hooks.js
import { env } from '$app/env/runtime';

const db = await connect(env.DATABASE_URL); // etc

// ...

Adapters that don't have the same design as Cloudflare Workers could call

server.init({
  env: process.env
});

or whatever when they first boot up.

Rich-Harris commented 2 years ago

(To be clear, I'm not suggesting any changes to how static variables are handled, just dynamic ones. And the names of all these modules are open to bikeshedding.)

f5io commented 2 years ago

I've just noticed something to consider for this specific topic that deals with the envPrefix.

The documentation states that you can supply your own envPrefix to use during the build, however, some sveltekit internals rely specifically on the VITE_ prefix. If you supply your own envPrefix then certain parts of sveltekit client runtime compile incorrectly.

https://github.com/sveltejs/kit/blob/948c980b0a8ee7756a307e3e9086adc878a144df/packages/kit/src/runtime/client/utils.js#L68-L116

I have managed to reproduce this issue with the following results:

with envPrefix: 'BUILD_' a build outputs the following:

function It() {
const {set: n, subscribe: e} = de(!1)
  , t = {}.VITE_SVELTEKIT_APP_VERSION;
let i;
async function c() {
    clearTimeout(i);
    const l = {}.VITE_SVELTEKIT_APP_VERSION_FILE
      , u = await fetch(`${Ze}/${l}`, {
        headers: {
            pragma: "no-cache",
            "cache-control": "no-cache"
        }
    });
    if (u.ok) {
        const {version: r} = await u.json()
          , s = r !== t;
        return s && (n(!0),
        clearTimeout(i)),
        s
    } else
        throw new Error(`Version check failed: ${u.status}`)
}
return {
    subscribe: e,
    check: c
}
}

with no envPrefix supplied, the build output is:

function Ot() {
const { set: n, subscribe: e } = de(!1),
t = '1658354847487';
let i;
async function c() {
clearTimeout(i);
const u = await fetch(`${Ze}/_app/version.json`, {
  headers: { pragma: 'no-cache', 'cache-control': 'no-cache' },
});
if (u.ok) {
  const { version: s } = await u.json(),
    r = s !== t;
  return r && (n(!0), clearTimeout(i)), r;
} else throw new Error(`Version check failed: ${u.status}`);
}
return { subscribe: e, check: c };
}

I'm happy to raise this as separate issue/bug, but maybe we should consider removing support for supplying your own envPrefix as otherwise it would require pre-process of the sveltekit internals.

edit: found this is related to... https://github.com/sveltejs/kit/issues/5584, ignore me

Rich-Harris commented 2 years ago

For everyone following along, there's now a PR open for this (thanks @tcc-sejohnson): https://github.com/sveltejs/kit/pull/5663

If anyone wants to offer feedback on the design we landed on, now is the time. I think it solves the various problems pretty well — I've been trying it out locally and enjoying it a lot.

madeleineostoja commented 2 years ago

Not sure if this is of any use since it's already shipped, but just wanted to give some feedback that I migrated a large project over to the new env variable handling, and I liked it a lot other than the /static and /dynamic paths for the modules. I found the names muddy and I had to read the docs closely to understand what they're doing rather than getting it intuitively out of the box.

I think just having $env/public and $env/private for the majority use-case (static), then handling dynamic variables separately (with $env/dynamic etc) would be much more ergonomic and easier to approach.

sveltejs / kit