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.

Author: ndhoule

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<ReturnType<NetlifyAPI['getEnvVars']>>[number] & {
   scopes: EnvelopeEnvVarScope[]
 }
-type EnvelopeEnvVarContext = NonNullable<NonNullable<EnvelopeEnvVar['values']>[number]['context']>
+
+type EnvelopeEnvVarContext = NonNullable<
+  | NonNullable<EnvelopeEnvVar['values']>[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) {

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')

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)
     })
   })
 

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', () => {