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

Use Process.env instead of Oxygen.env #1314

Open
wants to merge 3 commits into
base: v1.x-2022-07
Choose a base branch
from

Conversation

cartogram
Copy link
Contributor

Description

Fixes Shopify/hydrogen#1226 by populating process.env in our platforms/worker.ts and hydrogen middleware.

Additional context


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

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@cartogram cartogram marked this pull request as ready for review May 25, 2022 13:03
@github-actions

This comment has been minimized.

@jplhomer
Copy link
Contributor

Unfortunately, it appears that Vite will find/replace any usage of process.env if the SSR target is a webworker aka our worker bundle: https://github.com/vitejs/vite/blob/c68db4d7ad2c1baee41f280b34ae89a85ba0373d/packages/vite/src/node/plugins/define.ts#L51

This means all process.env instances are wiped from server components 😞

Marking this as blocked until we can find a workaround.

@jplhomer jplhomer added the blocked Can't continue until something else is fixed label May 26, 2022
@benjaminsehl
Copy link
Member

@wizardlyhel @blittle — should we close this PR in favour of import.meta?

@wizardlyhel
Copy link
Collaborator

@frandiox import.meta is a vite thing right?

@blittle
Copy link
Contributor

blittle commented Jul 13, 2022

@wizardlyhel @blittle — should we close this PR in favour of import.meta?

It is a vite thing. I'm wondering how it gets bundled / compiled, because runtimes (vercel, netlify, cloudflare, etc) aren't going to have it defined.

@frandiox
Copy link
Contributor

import.meta is a standard, but import.meta.env is added by Vite. It basically replaces any matches of this by its value at build time (i.e. inlines values in the bundle).

I remember suggesting adding a custom import.meta.secrets.XYZ for the private variables that would be compiled to process.env.XYZ or Oxygen.env.XYZ at build time. Not sure if feasible/desirable, though.

@blittle
Copy link
Contributor

blittle commented Jul 14, 2022

It's just really hard to know what to compile to, because each runtime does env variables slightly different. Which is why we are pushing for some sort of unification standard: wintercg/environment-metadata#1

I guess the CLI could have an option to compile to different runtime targets? Vercel, cloudflare, oxygen, etc?

@benjaminsehl
Copy link
Member

Now tracking in #1879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Can't continue until something else is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider dropping Oxygen.env and use process.env instead
6 participants