Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Consistent handling of environment variables #5663

Conversation

elliott-with-the-longest-name-on-github
Copy link
Contributor

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github commented Jul 21, 2022

Will close #4296 when finished.

This is a rough draft of the static environment variables solution. It adds a new config option: config.kit.env.publicPrefix, which defaults to PUBLIC_. Running svelte-kit sync will now dump all publicPrefix-prefixed environment variables to $app/env, and all variables not prefixed to $app/env/private. It also creates types for these files (for autocompletion).

Additionally, it adds a neat new feature that prevents importing $app/env/private into any client-side code by analyzing the import module graph during both dev and build. Initially, I had concerns around catching dynamic imports, but it seems this solution does a great job at that.

There are some rough edges -- I especially hate the amount of duplication between the Rollup and Vite algorithms in vite/utils.js. I'm sure there's a smarter way to structure that, but I've been screwing around with import graphs for too long right now to see it.

Update: Added support for the runtime variables part of the proposal.

Runtime variables are now exposed via import { env } from '$app/env/runtime'. env is typed as App.RuntimeEnv, which is just a convenience for developers (they can define what they expect the runtime env to be in order to provide intellisense when importing the module). This module is populated by calling set_env, which is itself called from server.init. Adapters are responsible for calling server.init when it is appropriate. For adapter-node (which I have updated as part of this PR for testing), this would be on startup.

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
Copy link

changeset-bot bot commented Jul 21, 2022

🦋 Changeset detected

Latest commit: a5e3a87

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-vercel Patch
@sveltejs/kit Patch
@sveltejs/adapter-auto 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

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github changed the title feat: Consistent handling of environment variables. feat: Consistent handling of environment variables Jul 21, 2022
@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

Added runtime variable support.

@campbell-hay
Copy link

I had to revert to version 70 (previous) because this version 71 was making my netlify function crash saying server.init did not exist or is undefined. Not sure how it is related but reverting back to 70 fixed the issue.

For ref, I am using the netlify adapter without any option, so all defaults.

@benmccann
Copy link
Member

@campbell-hay it sounds like you upgraded adapter-netlify, but not SvelteKit. You need to update both of them together

@campbell-hay
Copy link

@benmccann Oh good to know, my kit was 392 when it was not working with netlify adapter 71, now my kit 396. That means I could bump up the adapter to version 71 again then ?

@benmccann
Copy link
Member

Yes

camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Jul 27, 2022
- Update code to use new env var modules provided by Svelte Kit
- Remove obsolete /env endpoint for loading env vars
- Remove obsolete env store
- Add .env file for default public dynamic env vars
- Add PublicEnv interface for declaring the pubic env vars
- Use vite preview for running production build so .env is used for e2e tests
- See: sveltejs/kit#5663
camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Jul 27, 2022
- Update code to use new env var modules provided by Svelte Kit
- Remove obsolete /env endpoint for loading env vars
- Remove obsolete env store
- Add .env file for default public dynamic env vars
- Add PublicEnv interface for declaring the pubic env vars
- Use vite preview for running production build so .env is used for e2e tests
- See: sveltejs/kit#5663
@JordanShurmer
Copy link

Looks like the runtime variables are missing from the documentation.

Is that how we would load optional variables though? loading from the dynamic/static gives an error during the build if the variable doesn't exist.

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

https://kit.svelte.dev/docs/modules#$env-dynamic-private

Dynamic is runtime. You're likely importing it incorrectly -- dynamic only exports one object, env, containing the runtime environment vars.

@orlov-vo
Copy link

orlov-vo commented Aug 7, 2022

How to make conditional reading of environment values between private/public scopes based on browser variable?

I tried do it like this, but I've got Cannot import $env/dynamic/private.js into client-side code error

import { browser } from '$app/env';
import { PUBLIC_MY_VALUE } from '$env/static/public';

const getMyValue = () => {
    if (browser) return PUBLIC_MY_VALUE;
    return import('$env/dynamic/private').then(({ env }) => env.MY_VALUE);
};

@DevOfManyThings
Copy link
Contributor

@orlov-vo It defeats the point of private scoped variables if you can import them on the client, would you not be better off doing whatever logic you need to do in an endpoint with the private variable then just using the public one on the client?

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@orlov-vo

You cannot. We completely disallow imports of $env/*/private in .svelte files. You would leak those variables to the client. If you want to reference private variables, you must do so from a file that is guaranteed to only run on the server, such as an endpoint.

@RamiAwar
Copy link

RamiAwar commented Aug 11, 2022

@tcc-sejohnson How do you validate your env vars? I saw you mention that in your initial design proposal, I'm interested in doing that as well.

I think this should be in the docs too.

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@RamiAwar

That was something we decided to leave up to userland, though we solved some of it by accident. For the static modules, because you're importing the variables by name, if they're not defined at buildtime, your build will error. For dynamic vars, you can build an "env var validator" module, import it into hooks.ts, and call whatever validate method you want.

@RamiAwar
Copy link

@tcc-sejohnson Just noticed this when deploying with the new changes. That's awesome!

I feel like this should still be documented though imo, would be really useful to know that for static vars, build will error out if they're undefined.

@sasselin
Copy link

sasselin commented Aug 7, 2023

This feature work only in preview? Not with npm run dev?

JosephVolosin pushed a commit to NASA-AMMOS/aerie-ui that referenced this pull request Aug 20, 2024
- Update code to use new env var modules provided by Svelte Kit
- Remove obsolete /env endpoint for loading env vars
- Remove obsolete env store
- Add .env file for default public dynamic env vars
- Add PublicEnv interface for declaring the pubic env vars
- Use vite preview for running production build so .env is used for e2e tests
- See: sveltejs/kit#5663
JosephVolosin pushed a commit to NASA-AMMOS/aerie-ui that referenced this pull request Oct 21, 2024
- Update code to use new env var modules provided by Svelte Kit
- Remove obsolete /env endpoint for loading env vars
- Remove obsolete env store
- Add .env file for default public dynamic env vars
- Add PublicEnv interface for declaring the pubic env vars
- Use vite preview for running production build so .env is used for e2e tests
- See: sveltejs/kit#5663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent handling of environment variables