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(ssr): When available, leave process.env. accessible in SSR #4192

Closed
wants to merge 3 commits into from

Conversation

GrygrFlzr
Copy link
Member

@GrygrFlzr GrygrFlzr commented Jul 9, 2021

Closes #3176.

Description

Currently, Vite indiscriminately replaces process.env. with ({}). in order to prevent errors from dependencies which try to access process.env.VAR on the client side. This PR changes it so that it in SSR, it is accessible in Node-like environments which provide a process 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 reference process.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 use import.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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Jul 9, 2021
@Shinigami92 Shinigami92 self-requested a review July 9, 2021 09:23
Shinigami92
Shinigami92 previously approved these changes Jul 9, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a 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 🙂

@frandiox
Copy link
Contributor

frandiox commented Jul 9, 2021

This might break non-Node.js environments (such as Cloudflare Workers) since there is no global process variable there 🤔

I guess import.meta.env is still equivalent to process.env and is still being replaced, right?

Comment on lines +36 to +42
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).`
}
Copy link
Member Author

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.

Copy link
Contributor

@frandiox frandiox Jul 9, 2021

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.

#3151

@GrygrFlzr GrygrFlzr changed the title feat(ssr): Don't transform process.env. in SSR feat(ssr): When available, leave process.env. untouched in SSR Jul 9, 2021
@GrygrFlzr GrygrFlzr changed the title feat(ssr): When available, leave process.env. untouched in SSR feat(ssr): When available, leave process.env. accessible in SSR Jul 9, 2021
@GrygrFlzr GrygrFlzr marked this pull request as draft July 12, 2021 10:18
@GrygrFlzr
Copy link
Member Author

Temporarily marking as draft to look into using the ssr.target approach.

@bluwy
Copy link
Member

bluwy commented Oct 26, 2021

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.

@bluwy
Copy link
Member

bluwy commented Nov 3, 2021

I think we can close this with #5404 merged

@Niputi Niputi closed this Nov 3, 2021
mquandalle added a commit to mquandalle/mesaidesvelo that referenced this pull request Nov 15, 2021
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
@antfu antfu deleted the process-env-ssr branch December 24, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling server-side/sensitive/runtime variables
5 participants