fix: throw errors when function or isc-config cannot be loaded (#327)

netlify · Mar 7, 2023 · cdac30d · cdac30d
1 parent 331717d
commit cdac30d
Show file tree

Hide file tree

Showing 9 changed files with 161 additions and 81 deletions.
diff --git a/node/bundler.test.ts b/node/bundler.test.ts
@@ -297,7 +297,7 @@ test('Processes a function that imports a custom layer', async () => {
       path: '/func1',
     },
   ]
-  const layer = { name: 'layer:test', flag: 'edge-functions-layer-test' }
+  const layer = { name: 'https://edge-function-layer-template.netlify.app/mod.ts', flag: 'edge-functions-layer-test' }
   const result = await bundle([sourceDirectory], distPath, declarations, {
     basePath,
     configPath: join(sourceDirectory, 'config.json'),
@@ -333,7 +333,6 @@ test('Loads declarations and import maps from the deploy configuration', async (
     basePath,
     configPath: join(basePath, '.netlify', 'edge-functions', 'manifest.json'),
     internalSrcFolder: directories[1],
-    featureFlags: { edge_functions_config_export: true },
   })
   const generatedFiles = await fs.readdir(distPath)
 
@@ -370,7 +369,7 @@ test("Ignores entries in `importMapPaths` that don't point to an existing import
       helper: pathToFileURL(join(basePath, 'helper.ts')).toString(),
     },
     scopes: {
-      [pathToFileURL(join(sourceDirectory, 'func3')).toString()]: {
+      [pathToFileURL(join(sourceDirectory, 'func3/func3.ts')).toString()]: {
         helper: pathToFileURL(join(basePath, 'helper2.ts')).toString(),
       },
     },

diff --git a/node/bundler.ts b/node/bundler.ts
@@ -107,7 +107,9 @@ const bundle = async (
   await createFinalBundles([functionBundle], distDirectory, buildID)
 
   // Retrieving a configuration object for each function.
-  const functionsConfig = await Promise.all(functions.map((func) => getFunctionConfig(func, importMap, deno, logger)))
+  const functionsConfig = await Promise.all(
+    functions.map((func) => getFunctionConfig(func, importMap, deno, logger, featureFlags)),
+  )
 
   // Creating a hash of function names to configuration objects.
   const functionsWithConfig = functions.reduce(

diff --git a/node/config.test.ts b/node/config.test.ts
@@ -4,7 +4,7 @@ import { pathToFileURL } from 'url'
 
 import { deleteAsync } from 'del'
 import tmp from 'tmp-promise'
-import { test, expect, vi } from 'vitest'
+import { test, expect, vi, describe } from 'vitest'
 
 import { fixturesDir, useFixture } from '../test/util.js'
 
@@ -24,68 +24,99 @@ const importMapFile = {
 const invalidDefaultExportErr = (path: string) =>
   `Default export in '${path}' must be a function. More on the Edge Functions API at https://ntl.fyi/edge-api.`
 
-test('`getFunctionConfig` extracts configuration properties from function file', async () => {
-  const { path: tmpDir } = await tmp.dir()
-  const deno = new DenoBridge({
-    cacheDirectory: tmpDir,
-  })
-
-  const functions = [
-    // No config
-    {
-      expectedConfig: {},
-      name: 'func1',
-      source: `export default async () => new Response("Hello from function one")`,
-    },
-
-    // Empty config
-    {
-      expectedConfig: {},
-      name: 'func2',
-      source: `
+const functions = [
+  {
+    testName: 'no config',
+    expectedConfig: {},
+    name: 'func1',
+    source: `export default async () => new Response("Hello from function one")`,
+  },
+  {
+    testName: 'empty config',
+    expectedConfig: {},
+    name: 'func2',
+    source: `
         export default async () => new Response("Hello from function two")
 
         export const config = {}
       `,
-    },
-
-    // Config with the wrong type
-    {
-      expectedConfig: {},
-      name: 'func3',
-      source: `
+  },
+  {
+    testName: 'config with wrong type (throw)',
+    expectedConfig: {},
+    name: 'func3',
+    source: `
       export default async () => new Response("Hello from function two")
 
       export const config = () => ({})
     `,
-      userLog: /^'config' export in edge function at '(.*)' must be an object$/,
+    error:
+      /^The 'config' export in edge function at '(.*)' must be an object\. More on the Edge Functions API at https:\/\/ntl\.fyi\/edge-api\.$/,
+    featureFlags: {
+      edge_functions_invalid_config_throw: true,
     },
+  },
+  {
+    testName: 'config with wrong type (log)',
+    expectedConfig: {},
+    name: 'func3',
+    source: `
+      export default async () => new Response("Hello from function two")
 
-    // Config with a syntax error
-    {
-      expectedConfig: {},
-      name: 'func4',
-      source: `
+      export const config = () => ({})
+    `,
+    userLog: /^'config' export in edge function at '(.*)' must be an object$/,
+    featureFlags: {
+      edge_functions_invalid_config_throw: false,
+    },
+  },
+  {
+    testName: 'config with syntax error (throw)',
+    expectedConfig: {},
+    name: 'func4',
+    source: `
       export default async () => new Response("Hello from function two")
 
       export const config
     `,
-      userLog: /^Could not load edge function at '(.*)'$/,
+    error:
+      /^Could not load edge function at '(.*)'\. More on the Edge Functions API at https:\/\/ntl\.fyi\/edge-api\.$/,
+    featureFlags: {
+      edge_functions_invalid_config_throw: true,
     },
+  },
+  {
+    testName: 'config with syntax error (log)',
+    expectedConfig: {},
+    name: 'func4',
+    source: `
+      export default async () => new Response("Hello from function two")
 
-    // Config with `path`
-    {
-      expectedConfig: { path: '/home' },
-      name: 'func6',
-      source: `
+      export const config
+    `,
+    userLog: /^Could not load edge function at '(.*)'$/,
+    featureFlags: {
+      edge_functions_invalid_config_throw: false,
+    },
+  },
+  {
+    testName: 'config with `path`',
+    expectedConfig: { path: '/home' },
+    name: 'func6',
+    source: `
         export default async () => new Response("Hello from function three")
 
         export const config = { path: "/home" }
       `,
-    },
-  ]
+  },
+]
+describe('`getFunctionConfig` extracts configuration properties from function file', () => {
+  test.each(functions)('$testName', async (func) => {
+    const { path: tmpDir } = await tmp.dir()
+    const deno = new DenoBridge({
+      cacheDirectory: tmpDir,
+    })
 
-  for (const func of functions) {
     const logger = {
       user: vi.fn().mockResolvedValue(null),
       system: vi.fn().mockResolvedValue(null),
@@ -94,26 +125,31 @@ test('`getFunctionConfig` extracts configuration properties from function file',
 
     await fs.writeFile(path, func.source)
 
-    const config = await getFunctionConfig(
-      {
-        name: func.name,
-        path,
-      },
-      new ImportMap([importMapFile]),
-      deno,
-      logger,
-    )
-
-    expect(config).toEqual(func.expectedConfig)
-
-    if (func.userLog) {
-      expect(logger.user).toHaveBeenNthCalledWith(1, expect.stringMatching(func.userLog))
+    const funcCall = () =>
+      getFunctionConfig(
+        {
+          name: func.name,
+          path,
+        },
+        new ImportMap([importMapFile]),
+        deno,
+        logger,
+        func.featureFlags || {},
+      )
+
+    if (func.error) {
+      await expect(funcCall()).rejects.toThrowError(func.error)
+    } else if (func.userLog) {
+      await expect(funcCall()).resolves.not.toThrowError()
+      expect(logger.user).toHaveBeenCalledWith(expect.stringMatching(func.userLog))
     } else {
+      const config = await funcCall()
+      expect(config).toEqual(func.expectedConfig)
       expect(logger.user).not.toHaveBeenCalled()
     }
-  }
 
-  await deleteAsync(tmpDir, { force: true })
+    await deleteAsync(tmpDir, { force: true })
+  })
 })
 
 test('Loads function paths from the in-source `config` function', async () => {
@@ -188,17 +224,18 @@ test('Passes validation if default export exists and is a function', async () =>
 
   await fs.writeFile(path, func.source)
 
-  expect(async () => {
-    await getFunctionConfig(
+  await expect(
+    getFunctionConfig(
       {
         name: func.name,
         path,
       },
       new ImportMap([importMapFile]),
       deno,
       logger,
-    )
-  }).not.toThrow()
+      {},
+    ),
+  ).resolves.not.toThrow()
 
   await deleteAsync(tmpDir, { force: true })
 })
@@ -233,6 +270,7 @@ test('Fails validation if default export is not function', async () => {
     new ImportMap([importMapFile]),
     deno,
     logger,
+    {},
   )
 
   await expect(config).rejects.toThrowError(invalidDefaultExportErr(path))
@@ -269,6 +307,7 @@ test('Fails validation if default export is not present', async () => {
     new ImportMap([importMapFile]),
     deno,
     logger,
+    {},
   )
 
   await expect(config).rejects.toThrowError(invalidDefaultExportErr(path))

diff --git a/node/config.ts b/node/config.ts
@@ -7,6 +7,7 @@ import tmp from 'tmp-promise'
 import { DenoBridge } from './bridge.js'
 import { BundleError } from './bundle_error.js'
 import { EdgeFunction } from './edge_function.js'
+import { FeatureFlags } from './feature_flags.js'
 import { ImportMap } from './import_map.js'
 import { Logger } from './logger.js'
 import { getPackagePath } from './package_json.js'
@@ -39,7 +40,14 @@ const getConfigExtractor = () => {
   return configExtractorPath
 }
 
-export const getFunctionConfig = async (func: EdgeFunction, importMap: ImportMap, deno: DenoBridge, log: Logger) => {
+export const getFunctionConfig = async (
+  func: EdgeFunction,
+  importMap: ImportMap,
+  deno: DenoBridge,
+  log: Logger,
+  featureFlags: FeatureFlags,
+  // eslint-disable-next-line max-params
+) => {
   // The extractor is a Deno script that will import the function and run its
   // `config` export, if one exists.
   const extractorPath = getConfigExtractor()
@@ -72,7 +80,7 @@ export const getFunctionConfig = async (func: EdgeFunction, importMap: ImportMap
   )
 
   if (exitCode !== ConfigExitCode.Success) {
-    logConfigError(func, exitCode, stderr, log)
+    handleConfigError(func, exitCode, stderr, log, featureFlags)
 
     return {}
   }
@@ -86,20 +94,34 @@ export const getFunctionConfig = async (func: EdgeFunction, importMap: ImportMap
 
     return JSON.parse(collectorData) as FunctionConfig
   } catch {
-    logConfigError(func, ConfigExitCode.UnhandledError, stderr, log)
+    handleConfigError(func, ConfigExitCode.UnhandledError, stderr, log, featureFlags)
 
     return {}
   } finally {
     await collector.cleanup()
   }
 }
 
-const logConfigError = (func: EdgeFunction, exitCode: number, stderr: string, log: Logger) => {
+const handleConfigError = (
+  func: EdgeFunction,
+  exitCode: number,
+  stderr: string,
+  log: Logger,
+  featureFlags: FeatureFlags,
+  // eslint-disable-next-line max-params
+) => {
   switch (exitCode) {
     case ConfigExitCode.ImportError:
-      log.user(`Could not load edge function at '${func.path}'`)
       log.user(stderr)
-
+      if (featureFlags.edge_functions_invalid_config_throw) {
+        throw new BundleError(
+          new Error(
+            `Could not load edge function at '${func.path}'. More on the Edge Functions API at https://ntl.fyi/edge-api.`,
+          ),
+        )
+      } else {
+        log.user(`Could not load edge function at '${func.path}'`)
+      }
       break
 
     case ConfigExitCode.NoConfig:
@@ -108,13 +130,27 @@ const logConfigError = (func: EdgeFunction, exitCode: number, stderr: string, lo
       break
 
     case ConfigExitCode.InvalidExport:
-      log.user(`'config' export in edge function at '${func.path}' must be an object`)
-
+      if (featureFlags.edge_functions_invalid_config_throw) {
+        throw new BundleError(
+          new Error(
+            `The 'config' export in edge function at '${func.path}' must be an object. More on the Edge Functions API at https://ntl.fyi/edge-api.`,
+          ),
+        )
+      } else {
+        log.user(`'config' export in edge function at '${func.path}' must be an object`)
+      }
       break
 
     case ConfigExitCode.SerializationError:
-      log.user(`'config' object in edge function at '${func.path}' must contain primitive values only`)
-
+      if (featureFlags.edge_functions_invalid_config_throw) {
+        throw new BundleError(
+          new Error(
+            `The 'config' object in the edge function at '${func.path}' must contain primitive values only. More on the Edge Functions API at https://ntl.fyi/edge-api.`,
+          ),
+        )
+      } else {
+        log.user(`'config' object in edge function at '${func.path}' must contain primitive values only`)
+      }
       break
 
     case ConfigExitCode.InvalidDefaultExport:

diff --git a/node/feature_flags.ts b/node/feature_flags.ts
@@ -1,9 +1,10 @@
-const defaultFlags: Record<string, boolean> = {
+const defaultFlags = {
   edge_functions_fail_unsupported_regex: false,
+  edge_functions_invalid_config_throw: false,
 }
 
 type FeatureFlag = keyof typeof defaultFlags
-type FeatureFlags = Record<FeatureFlag, boolean>
+type FeatureFlags = Partial<Record<FeatureFlag, boolean>>
 
 const getFlags = (input: Record<string, boolean> = {}, flags = defaultFlags): FeatureFlags =>
   Object.entries(flags).reduce(

diff --git a/node/server/server.ts b/node/server/server.ts
@@ -84,7 +84,7 @@ const prepareServer = ({
     let functionsConfig: FunctionConfig[] = []
 
     if (options.getFunctionsConfig) {
-      functionsConfig = await Promise.all(functions.map((func) => getFunctionConfig(func, importMap, deno, logger)))
+      functionsConfig = await Promise.all(functions.map((func) => getFunctionConfig(func, importMap, deno, logger, {})))
     }
 
     const success = await waitForServer(port, processRef.ps)