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

Polyfill process.env on Netlify edge #1108

Closed
wants to merge 3 commits into from

Conversation

iainmerrick
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

The only way to read environment variables at runtime on Netlify edge is via their Netlify.env.get function. As well as being Netlify-specific, this is awkward to use from TypeScript as it doesn't seem to have correct type declarations.

What is the new behavior?

Environment variables can be read via process.env.

This matches the default behavior on Vercel's edge, and the Next.js middleware on Netlify edge.

Other information

I haven't included a test in this PR, but I made a test site in a separate repo (https://github.com/iainmerrick/solid-start-edge-test/). Here's a route that reads process.env (using this PR):

@iainmerrick
Copy link
Contributor Author

iainmerrick commented Nov 2, 2023

Oops, this is that was not quite correct as Rollup will reorder my code so the polyfill comes after the imports. This means process.env works inside event handlers, but not in top-level code. Will fix. Fixed.

@ryansolid
Copy link
Member

In setting up for SolidStarts next Beta Phase built on Nitro and Vinxi we are closing all PRs/Issues that will not be merged due to the system changing. If you feel your issue was closed in mistake. Feel free to re-open it after updating/testing against 0.4.x release. Thank you for your patience.

See #1139 for more details.

@ryansolid ryansolid closed this Dec 18, 2023
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

Successfully merging this pull request may close these issues.

2 participants