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

process.browser etc #8

Closed
Rich-Harris opened this issue Sep 18, 2020 · 14 comments
Closed

process.browser etc #8

Rich-Harris opened this issue Sep 18, 2020 · 14 comments

Comments

@Rich-Harris
Copy link
Member

The Sapper template allows you to write process.browser to determine if you're in the browser or in Node. Not sure how to do that here. Snowpack has import.meta.env which feels like the way you're supposed to go, though I'm not sure how to differentiate between SSR and not. Perhaps the isSSR boolean that gets passed around could also be used to populate import.meta.env.SSR?

@antony
Copy link
Member

antony commented Oct 13, 2020

import.meta.env is absolutely the correct approach. There's actually an open PR to backport this to Sapper.

@Conduitry
Copy link
Member

Should import.meta.env be reserved for things that really are environment variables? In the Rollup config for some Sapper projects I've written, I settled on the pattern 'use process.env.whatever for replacements that are environment variables, use process.whatever for other replacements, like say pkg.version' - and I think having some way to tell these apart is useful. I'd be confused if a project had process.env.something in it and I couldn't affect it by that build-time env var. Would import.meta.env.something tend to convey the same thing?

@Rich-Harris Rich-Harris transferred this issue from another repository Oct 15, 2020
@FredKSchott
Copy link
Contributor

In Snowpack, we're trying to be very careful not to extend import.meta too much, since it's still not consensus what belongs on import.meta and what belongs as an import (import {WHATEVER} from 'snowpack:env', or something similar). However, we've been supporting import.meta.env since day one and now most other ESM build tools/envs/bundlers are following suit, so we're feeling pretty good about the direction. Webpack 5 just added their own HMR support on import.meta, for example.

If you check https://www.snowpack.dev/#environment-variables, you'll see we already have support for import.meta.env.MODE as a less-node-specific replacement for process.env.NODE_ENV. I think something like this makes sense here as well.

Ideas (any of which I'd be happy with):

  • my favorite: import.meta.env.TARGET = 'node' | 'web' | 'electron'
    • matches webpack
    • alt: import.meta.env.ENV or import.meta.env.RUNTIME
  • import.meta.env.SSR = true | false;
    • less flexible
    • alt: import.meta.env.IS_SSR or import.meta.env.BROWSER

@Rich-Harris
Copy link
Member Author

I noticed in the Snowpack 2.15 changelog that it's now import.meta.env.SSR: FredKSchott/snowpack#1345.

I couldn't fully test it because as soon as you add import.meta, Snowpack injects the HMR client which doesn't work in Node. I've opened a discussion: FredKSchott/snowpack#1370

@Rich-Harris
Copy link
Member Author

Just tested this with Snowpack 2.15.1, and it works:

<h1>Hello {import.meta.env.SSR ? 'server' : 'client'}!</h1>

@benmccann
Copy link
Member

Will all environment variables get put in import meta.env and will that get shipped to the browser? Lots of people put credentials in environment variables, so I just want to make sure we don't accidentally expose those

@lukeed
Copy link
Member

lukeed commented Oct 23, 2020

@benmccann yes and it's my biggest concern with snowpack currently. And since we're doing server and browser during snowpack, we really really cannot put server-side secrets anywhere within Snowpack's reach. Or at least make sure that it's shaken away during the 2nd optimization pass.

@FredKSchott
Copy link
Contributor

FredKSchott commented Oct 30, 2020

@lukeed that's incorrect, we protect for this by only supporting SNOWPACK_PUBLIC_* env variables in the frontend build. You can see our docs here: https://www.snowpack.dev/#environment-variables.%20%20

Next.js, Create React App, etc. all have similar ideas of only allowing "special prefixed" env variables to be added to the project for this exact reason. For ex, Next.js uses NEXT_PUBLIC_*: https://nextjs.org/docs/basic-features/environment-variables#exposing-environment-variables-to-the-browser

It looks like Next.js supports all process.env secrets in SSR mode but that then get removed in the frontend build. I'd be +1 for adding a similar workflow to Snowpack.

@lukeed
Copy link
Member

lukeed commented Oct 30, 2020

You're right that in a browser-only development space, this is easy to manage, to be aware of, and to workaround. It's generally not a problem. The prefix is the extra, much-needed callout that says "hey, this is public~!"

But when juggling a dual-environment setup, which is what we're talking about, it doesn't fit in the same nice box.

In a SSR context, all this really means is that a user goes from using API_TOKEN to SNOWPACK_PUBLIC_API_TOKEN in order to pass the necessary value(s) through Snowpack so that the server code can read them out. They're still very much there in the output. I'm looking at them.

Quick example – on server-side preload, you interface directly with a mysql client. To do so, you need to pass in HOST/USER/etc values, which is typically done through ENV. Great – that's fine – but the burden is on the user to be sure that those values never ever get loaded via a client-side import; or as in this case, it requires a svelte/kit to double-process the files and guarantee that server-side code is built & kept completely separately from the client-side bundle. But, yes, the sensitive ENV variables are in the (server) output code. They have to be – because of the rules/guidelines for how Snowpack expects/allows ENV to be defined.

@FredKSchott
Copy link
Contributor

Just to make sure I'm following, do you have the same concerns with how Next.js currently does this? https://nextjs.org/docs/basic-features/environment-variables (all secrets available in SSR mode, all non-PUBLIC secrets removed from the frontend build).

@lukeed
Copy link
Member

lukeed commented Oct 30, 2020

No, because they're directly in control of the build pipelines and can guarantee that the server-side secrets are removed/kept separate from the client build.

But Next.js is not comparable to Snowpack. Snowpack will write the secrets into the output files (which is fine), but it requires a sufficiently aware user and/or a Next.js-like framework (eg, svelte kit) to take responsibility for ensuring that happens correctly.

@lukeed
Copy link
Member

lukeed commented Oct 30, 2020

In Next, Rollup/webpack/whatever is writing secrets into output files too. It's fine – just something or someone has to make sure they're juggling and/or cleaning that up correctly.

My "concern" with Snowpack is only that it will eventually be approached by non-SvelteKit developers, who won't be aware enough of this "gotcha" (too strong of a word), and so when they go to roll their own solution, they'll leak server-only ENVs everywhere by mistake.

@FredKSchott
Copy link
Contributor

I think that's a misunderstanding of how SSR works in Snowpack (at least as of 2.15). Snowpack now has an idea of true SSR mode (which svelte/kit uses internally) so that if we wanted to we actually can control / guarantee the server-side secrets are removed/kept separate from the client build while still making them available in SSR mode.

// in SSR mode:
import.meta.env = { ... full set of env vars ... }

// in non-SSR mode (the frontend build):
import.meta.env = { ... only SNOWPACK_PUBLIC_* env vars... }

@lukeed
Copy link
Member

lukeed commented Oct 30, 2020

Ah cool, glad to hear! I'll check it out again. Ran into that problem time & time again when setting up the custom Skypack rig.

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

No branches or pull requests

6 participants