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

[hydrogen] Polyfill Oxygen global in Hydrogen #8413

Closed
wants to merge 1 commit into from

Conversation

frandiox
Copy link

Related Issues

It's currently not possible to access environment variables in Hydrogen projects deployed to Vercel. The reason is that Hydrogen currently relies on a global Oxygen.env that needs to be polyfilled in the platform. This PR adds the global object using process.env, which is mentioned in the docs as the way to access environment variables in Edge Functions.

@TooTallNate Can you double check if this is correct? I'm not quite sure process.env is the right thing here even if it's mentioned in the docs 😅

📋 Checklist

Tests

  • The code changed/added as part of this PR has been covered with tests
  • All tests pass locally with yarn test-unit

Code Review

  • This PR has a concise title and thorough description useful to a reviewer
  • Issue from task tracker has a link to this PR

Copy link
Contributor

@EndangeredMassa EndangeredMassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test that shows the environment variables being used? You can make a new fixture, set env vars, make sure they are used in the fixture app somewhere, then leverage a vercel.json probe to test for that value.

If not, no worries! We can add a test for this.

@@ -5,8 +5,10 @@ import indexTemplate from '__RELATIVE__/dist/client/index.html?raw';
import { ReadableStream } from 'web-streams-polyfill/ponyfill';
Object.assign(globalThis, { ReadableStream });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to do globalThis changes in one clear place up top, like:

Object.assign(globalThis, {
  // ReadableStream is bugged in Vercel Edge, overwrite with polyfill
  ReadableStream,

  // Hydrogen exposes env vars through `Oxygen.env`
  Oxygen: {
    env: process.env
  }
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this idea. Global is global and it's best outside of a function.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 hey I made this change here #8496 if you want to take a look

@styfle styfle changed the title Polyfill Oxygen global in Hydrogen [hydrogen] Polyfill Oxygen global in Hydrogen Aug 24, 2022
@styfle
Copy link
Member

styfle commented Aug 31, 2022

Closing in favor of #8496

@styfle styfle closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants