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

Reload environment variables in development #997

Merged
merged 8 commits into from
Jul 18, 2023
Merged

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Jun 9, 2023

Builds on top of https://github.com/Shopify/mini-oxygen/pull/458 to allow reloading environment variables when .env file changes.

This also changes the behavior of the combinedEnvironmentVariables utility to avoid blocking the initial build.

Not sure what to do with the logs. Right now it logs "Injecting variables to MiniOxygen: ....." every time the .env file is updated, which is not incorrect but might be too much output. Perhaps we could diff and only show the stuff that has changed.
Also, right now this is downloading remote vars again on file save... Perhaps we should assume the remote variables don't change and we only read the local .env on file save?

@frandiox frandiox requested a review from graygilmore June 9, 2023 13:31
@frandiox frandiox mentioned this pull request Jun 9, 2023
5 tasks
@github-actions github-actions bot had a problem deploying to preview June 9, 2023 13:38 Failure
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what to do with the logs. Right now it logs "Injecting variables to MiniOxygen: ....." every time the .env file is updated, which is not incorrect but might be too much output. Perhaps we could diff and only show the stuff that has changed.

I've bee going back and forth on where logs should go. Maybe it'd be easier if certain components were broken down even further into functions so that things could be more reuseable?

On save I think it'd be better to have something like "Change detected; reloading environment variables". I like the idea of a diff to shorten the output but might be more work than it's worth.

Copy link
Contributor

Choose a reason for hiding this comment

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

How often will someone realistically be changing the .env file? If I change it, I think it's nice to know that the server is getting those changes. So I think the feedback is good and I don't imagine it changing all that often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, keeping the same logs for now 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, right now this is downloading remote vars again on file save... Perhaps we should assume the remote variables don't change and we only read the local .env on file save?

I actually kind of love that it fetches them again but I wonder if that would be considered unexpected...

@frandiox frandiox marked this pull request as ready for review July 6, 2023 14:38
@frandiox frandiox requested a review from a team July 6, 2023 14:38
@github-actions github-actions bot had a problem deploying to preview July 6, 2023 14:43 Failure
@frandiox frandiox merged commit 63d1726 into 2023-04 Jul 18, 2023
@frandiox frandiox deleted the fd-reload-env-dev branch July 18, 2023 13:59
FrcPpe pushed a commit to FrcPpe/hydrogen that referenced this pull request Aug 13, 2023
* Load env vars in parallel to initial build; Do not throw on env var errors

* Reload env variables when project is linked to storefront

* Wrap all calls in try-catch

* Resolve variables in parallel and avoid crashing on fetch fail

* Add .env to Remix watcher in templates

* Changesets
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.

3 participants