-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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(ssr): When available, leave process.env.
accessible in SSR
#4192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a code review, not a "it-works" review 🙂
This might break non-Node.js environments (such as Cloudflare Workers) since there is no global I guess |
const processEnv = ssr | ||
? { | ||
// account for non-node environments like v8 | ||
'process.env.': `(typeof process === 'undefined' ? {} : process.env).`, | ||
'global.process.env.': `(typeof global.process === 'undefined' ? {} : global.process.env).`, | ||
'globalThis.process.env.': `(typeof globalThis.process === 'undefined' ? {} : globalThis.process.env).` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm extremely unhappy with this runtime check, but as @frandiox points out, potential target environments like Cloudflare Workers and Deno Deploy do not have process
as it is a node-ism.
Information about the target environment would need to be available at build time in order to remove the runtime check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GrygrFlzr I believe there's a build variable in Vite, ssr.target
, which you might be able to use for this. It can have webworker
value. Therefore, the check would happen at build-time instead of run-time like in this code 🤔
Not sure if it's easy to access that variable though.
process.env.
in SSRprocess.env.
untouched in SSR
process.env.
untouched in SSRprocess.env.
accessible in SSR
Temporarily marking as draft to look into using the |
FYI I've made a new PR #5404 that supersedes this. I think we can close this. I've also contacted @GrygrFlzr beforehand and he mentioned he was busy recently. |
I think we can close this with #5404 merged |
Ouvre une issue sur Github avec le message de l'utilisateur J'ai été obligé d'utiliser la beta de vite pour pouvoir exposer les variables d'environnement côté serveur, cf. vitejs/vite#4192
Closes #3176.
Description
Currently, Vite indiscriminately replaces
process.env.
with({}).
in order to prevent errors from dependencies which try to accessprocess.env.VAR
on the client side. This PR changes it so that it in SSR, it is accessible in Node-like environments which provide aprocess
global binding.With this PR, it is now possible for server-side code running on node to access
process.env.VAR
normally as it would in any other node environment.Additional context
This PR does not change any security implications as you were already able to work around the aggressive transform using
process.env['KEY']
. It is the responsibility of the developer/frameworks not to include secrets in anything that will be available to the end user, especially as different frameworks have different ways to establish "server" code, "client" code, and isomorphic code, which is outside of Vite's scope and ability to identify.It is the developer's responsibility to use
process.env
strictly in server-only code, as it does not exist on the client. Isomorphic code which referenceprocess.env
will technically resolve during server render, but will fail upon hydration, and should similarly be avoided. Both client-only and isomorphic code should continue to useimport.meta.env.VITE_
for insensitive data resolvable at build time.Vite's
.env
file loading feature is not within the scope of this PR, and has been left untouched.The special case of
process.env.NODE_ENV
introduced by #3703 has not been altered.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).