-
Notifications
You must be signed in to change notification settings - Fork 433
fix: load correct environment variables in Preview Server context #7585
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
fix: load correct environment variables in Preview Server context #7585
Conversation
📊 Benchmark resultsComparing with 99a2f05
|
5969270 to
f145479
Compare
Related issue: netlify/cli#7585
ff6a7d6 to
bc14e8e
Compare
| env[BLOBS_CONTEXT_VARIABLE] = { sources: ['internal'], value: encodeBlobsContext(blobsContext) } | ||
|
|
||
| if (!options.offline && !options.offlineEnv) { | ||
| if (!(options.offline || options.offlineEnv)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I find first to easier to read, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A matter of taste, but I find "if neither one is true" to be easier to reason about than "if not x and not y." (Fewer logical operations to hold in your head, too.)
594a391 to
b02d96f
Compare
Related issue: netlify/cli#7585
This changeset fixes environment-variable loading a Preview Server context. Previously, we were missing `dev-server` from the CLI's list of supported contexts. Because of a variable that (unnecessarily) mixes the concept of branches and contexts, when we run into an unsupported context, we assume we must be running a branch preview, and try to find variables for the branch named `dev-server`. For the majority of users, variables for that branch won't exist, and they'll end up with an unexpectedly empty variable. Why do we assume it's a branch preview when we have enough information available to us to know it's not one? Why do we throw away the type information that would have caught this? Why do we load environment variables from the API in the CLI even though `@netlify/config` already does this? These are the mysteries of the universe.
b02d96f to
bad236b
Compare
Related issue: netlify/cli#7585
This changeset fixes environment-variable loading a Preview Server context.
Previously, we were missing
dev-serverfrom the CLI's list ofsupported contexts. Because of a variable that (unnecessarily) mixes the
concept of branches and contexts, when we run into an unsupported
context, we assume we must be running a branch preview, and try to find
variables for the branch named
dev-server. For the majority of users,variables for that branch won't exist, and they'll end up with an
unexpectedly empty variable.
Why do we assume it's a branch preview when we have enough information
available to us to know it's not one? Why do we throw away the type
information that would have caught this? Why do we load environment
variables from the API in the CLI even though
@netlify/configalreadydoes this?
These are the mysteries of the universe.