From 6d75c4592144a9456a678ed9b1a9816bb0f9779b Mon Sep 17 00:00:00 2001 From: Nathan Houle Date: Tue, 26 Aug 2025 14:01:33 -0700 Subject: [PATCH 1/2] fix: load correct environment variables in Preview Server context 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. --- src/utils/env/index.ts | 16 ++++++++++++++-- tests/integration/commands/env/env-set.test.ts | 2 +- tests/integration/commands/env/env-unset.test.ts | 2 +- tests/unit/utils/env/index.test.ts | 9 +++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/utils/env/index.ts b/src/utils/env/index.ts index 5da8f95038f..1fea38caefd 100644 --- a/src/utils/env/index.ts +++ b/src/utils/env/index.ts @@ -8,7 +8,7 @@ import type { SiteInfo, EnvironmentVariableSource } from '../../utils/types.js' * These all match possible `context` values returned by the Envelope API. * Note that a user may also specify a branch name with the special `branch:my-branch-name` format. */ -export const SUPPORTED_CONTEXTS = ['all', 'production', 'deploy-preview', 'branch-deploy', 'dev'] as const +export const SUPPORTED_CONTEXTS = ['all', 'production', 'deploy-preview', 'branch-deploy', 'dev', 'dev-server'] as const /** * Additional aliases for the user-provided env `context` option. */ @@ -30,7 +30,13 @@ type EnvelopeEnvVarScope = type EnvelopeEnvVar = Awaited>[number] & { scopes: EnvelopeEnvVarScope[] } -type EnvelopeEnvVarContext = NonNullable[number]['context']> + +type EnvelopeEnvVarContext = NonNullable< + | NonNullable[number]['context'] + // TODO(ndhoule): Netlify API is incorrect - Update OpenAPI types with this context type .. + | 'dev-server' +> + export type EnvelopeEnvVarValue = { /** * The deploy context of the this env var value @@ -104,6 +110,12 @@ export const getValueForContext = ( ): EnvelopeEnvVarValue | undefined => { const isSupportedContext = (SUPPORTED_CONTEXTS as readonly string[]).includes(contextOrBranch) if (!isSupportedContext) { + // FIXME(ndhoule): If it's not a supported context, we just assume this is a branch deploy. This + // means that if you ever add a deploy context but forget to add it to SUPPORTED_CONTEXTS, we'll + // just load the wrong environment variables. (This bug is not theoretical: it's why I'm writing + // this comment.) We should instead pass a `context` and optional `branch` parameter to this + // function rather than mix the two concepts, and we should fail when an unsupported context is + // provided. const valueMatchingAsBranch = values.find((val) => val.context_parameter === contextOrBranch) // This is a `branch` context, which is an override, so it takes precedence if (valueMatchingAsBranch != null) { diff --git a/tests/integration/commands/env/env-set.test.ts b/tests/integration/commands/env/env-set.test.ts index 4e1c4fd2f0c..b0ae1f948a0 100644 --- a/tests/integration/commands/env/env-set.test.ts +++ b/tests/integration/commands/env/env-set.test.ts @@ -196,7 +196,7 @@ describe('env:set command', async () => { (request) => request.method === 'PUT' && request.path === '/api/v1/accounts/test-account/env/OTHER_VAR', ) expect(putRequest).toHaveProperty('body.is_secret', true) - expect(putRequest).toHaveProperty('body.values.length', 4) + expect(putRequest).toHaveProperty('body.values.length', 5) expect(putRequest).toHaveProperty('body.values[0].context', 'production') expect(putRequest).toHaveProperty('body.values[0].value', 'envelope-all-value') expect(putRequest).toHaveProperty('body.values[1].context', 'deploy-preview') diff --git a/tests/integration/commands/env/env-unset.test.ts b/tests/integration/commands/env/env-unset.test.ts index b85d790ff9e..41a0ae700cc 100644 --- a/tests/integration/commands/env/env-unset.test.ts +++ b/tests/integration/commands/env/env-unset.test.ts @@ -72,7 +72,7 @@ describe('env:unset command', async () => { (request) => request.method === 'PATCH' && request.path === '/api/v1/accounts/test-account/env/OTHER_VAR', ) - expect(patchRequests).toHaveLength(3) + expect(patchRequests).toHaveLength(4) }) }) diff --git a/tests/unit/utils/env/index.test.ts b/tests/unit/utils/env/index.test.ts index d54e5a5536a..3e1de1e1ca4 100644 --- a/tests/unit/utils/env/index.test.ts +++ b/tests/unit/utils/env/index.test.ts @@ -23,9 +23,14 @@ test('should return a matching value from a given context', () => { context: 'dev', value: 'bar', }, + { + context: 'dev-server', + value: 'bar', + }, ]) - const result = getValueForContext(values, 'dev') - expect(result).toHaveProperty('value', 'bar') + expect(getValueForContext(values, 'production')).toHaveProperty('value', 'foo') + expect(getValueForContext(values, 'dev')).toHaveProperty('value', 'bar') + expect(getValueForContext(values, 'dev-server')).toHaveProperty('value', 'bar') }) test('should return a value from the `branch` context with a matching `context_parameter` given a branch', () => { From bad236b2dc0d0b794bfcfaa3cebbb1021cd2691d Mon Sep 17 00:00:00 2001 From: Nathan Houle Date: Tue, 26 Aug 2025 14:40:45 -0700 Subject: [PATCH 2/2] refactor: simplify offline-check conditional --- src/commands/dev/dev.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/dev/dev.ts b/src/commands/dev/dev.ts index a87d51ef4ad..6102e652b38 100644 --- a/src/commands/dev/dev.ts +++ b/src/commands/dev/dev.ts @@ -137,7 +137,7 @@ export const dev = async (options: OptionValues, command: BaseCommand) => { env[BLOBS_CONTEXT_VARIABLE] = { sources: ['internal'], value: encodeBlobsContext(blobsContext) } - if (!options.offline && !options.offlineEnv) { + if (!(options.offline || options.offlineEnv)) { env = await getEnvelopeEnv({ api, context: options.context, env, siteInfo }) log(`${NETLIFYDEVLOG} Injecting environment variable values for ${chalk.yellow('all scopes')}`) }