Skip to content

feat: inject environment variables from .env files into edge functions locally#5620

Merged
kodiakhq[bot] merged 6 commits into
mainfrom
env-edge
Apr 17, 2023
Merged

feat: inject environment variables from .env files into edge functions locally#5620
kodiakhq[bot] merged 6 commits into
mainfrom
env-edge

Conversation

@danez
Copy link
Copy Markdown
Contributor

@danez danez commented Apr 12, 2023 •
edited
Loading

Summary

This allows any environment variables that are coming from dot-env files (.env) to be accessed in edge functions.

Similar to what is described here: https://docs.netlify.com/configure-builds/environment-variables/#build-locally

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@danez danez self-assigned this Apr 12, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 12, 2023 •
edited
Loading

📊 Benchmark results

Comparing with f30d409

Package size: 302 MB

(no change)

^  302 MB  302 MB  302 MB  302 MB  302 MB  302 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-5     T-4     T-3     T-2     T-1      T
Legend

@danez danez changed the title feat: inject environment variables from .env files into edge function… feat: inject environment variables from .env files into edge functions locally Apr 12, 2023
@danez danez requested a review from klavavej April 12, 2023 14:56
@danez danez marked this pull request as ready for review April 12, 2023 15:00
@danez danez requested a review from a team April 12, 2023 15:00
danez
danez commented Apr 12, 2023
View reviewed changes
Comment thread src/utils/dev.mjs
* @param {Record<string, { sources: string[], value: string}>} env
* @return {void}
*/
export const injectEnvVariables = (env) => {
Copy link
Copy Markdown
Contributor Author

@danez danez Apr 12, 2023

Choose a reason for hiding this comment

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

I made this function to only inject variables into process.env and split out all the dotenv stuff into the function above.

danez
danez commented Apr 12, 2023
View reviewed changes
Comment thread src/commands/dev/dev.mjs

await injectEnvVariables({ devConfig, env, site })
env = await addDotEnvVariables({ devConfig, env, site })
injectEnvVariables(env)
Copy link
Copy Markdown
Contributor Author

@danez danez Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

addDotEnvVariables now mutates env here and adds the variables from the dot-env files to it, so that later one we can use them in edge-functions. Previously dotenv variables were only added to process.env directly.

danez
danez commented Apr 12, 2023
View reviewed changes
Comment thread tests/integration/100.command.dev.test.cjs Outdated
@@ -1,4 +1,4 @@
// Handlers are meant to be async outside tests
const { platform } = require('os')
Copy link
Copy Markdown
Contributor Author

@danez danez Apr 12, 2023

Choose a reason for hiding this comment

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

I couldn't figure out what this comment means or where it applies to :D

@klavavej
Copy link
Copy Markdown

klavavej commented Apr 12, 2023

@danez - I'm not sure what part of this PR you want me to review. I took a look through and couldn't identify any changes that will impact what's shown in the cli docs at https://deploy-preview-5620--cli.netlify.app/

Could you point me to the part of this PR that needs my review?

I do see that docs.netlify.com documents some limitations around this behavior that will need to be updated/removed (on the Environment variables and functions page and the Edge Functions limits page)- I can open a PR to get the ball rolling on those changes.

@danez
Copy link
Copy Markdown
Contributor Author

danez commented Apr 12, 2023

@danez - I'm not sure what part of this PR you want me to review. I took a look through and couldn't identify any changes that will impact what's shown in the cli docs at https://deploy-preview-5620--cli.netlify.app/

Could you point me to the part of this PR that needs my review?

I do see that docs.netlify.com documents some limitations around this behavior that will need to be updated/removed (on the Environment variables and functions page and the Edge Functions limits page)- I can open a PR to get the ball rolling on those changes.

Sorry, i didn't mean to ask for a review. I just wanted to let you know that this is something that is changing which we might want to document :)

eduardoboucas
eduardoboucas previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

Nice! Left a couple of comments but nothing blocking.

Comment thread src/commands/dev/dev-exec.mjs Outdated
Comment thread src/lib/edge-functions/registry.mjs
@danez danez force-pushed the env-edge branch 3 times, most recently from 3494edb to 252a386 Compare April 14, 2023 12:53
@danez danez requested review from a team and eduardoboucas and removed request for klavavej April 17, 2023 11:47
khendrikse
khendrikse previously approved these changes Apr 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@khendrikse khendrikse left a comment

Choose a reason for hiding this comment

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

Looks good

@danez danez added the automerge Add to Kodiak auto merge queue label Apr 17, 2023
@danez danez requested a review from khendrikse April 17, 2023 12:59
@kodiakhq kodiakhq Bot merged commit 9066166 into main Apr 17, 2023
@kodiakhq kodiakhq Bot deleted the env-edge branch April 17, 2023 13:10
@token-generator-app token-generator-app Bot mentioned this pull request Apr 17, 2023
Merged
@CASABECI CASABECI mentioned this pull request May 8, 2024
Open
@MaxMood96 MaxMood96 mentioned this pull request May 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khendrikse khendrikse khendrikse approved these changes

@eduardoboucas eduardoboucas Awaiting requested review from eduardoboucas

Assignees

@danez danez

Labels

automerge Add to Kodiak auto merge queue needs docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danez @klavavej @eduardoboucas @khendrikse