feat: move non-route related ef configs to function_config in manifest (

#348) * feat: first work on moving ef configs that are not route-related to function_config * feat: add comments for backwards-compatibility * test: fix test and refactor a few things * chore: fix typos en rename some things * feat: add failsafe for internal in-source configurations * chore: make name more explicit as functionName in mergeWithDeclarationConfig * chore: remove hasAnyConfigValues * test: fix test * Update node/bundler.ts add better comment Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com> --------- Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
netlify · Mar 24, 2023 · c7b7042 · c7b7042
1 parent c85a861
commit c7b7042
Show file tree

Hide file tree

Showing 9 changed files with 202 additions and 87 deletions.
diff --git a/node/bundler.test.ts b/node/bundler.test.ts
@@ -321,7 +321,7 @@ test('Processes a function that imports a custom layer', async () => {
   await cleanup()
 })
 
-test('Loads declarations and import maps from the deploy configuration', async () => {
+test('Loads declarations and import maps from the deploy configuration and in-source config', async () => {
   const { basePath, cleanup, distPath } = await useFixture('with_deploy_config')
   const declarations: Declaration[] = [
     {
@@ -342,18 +342,24 @@ test('Loads declarations and import maps from the deploy configuration', async (
 
   const manifestFile = await fs.readFile(resolve(distPath, 'manifest.json'), 'utf8')
   const manifest = JSON.parse(manifestFile)
-  const { routes, bundles, function_config: functionConfig } = manifest
-
+  const { bundles, function_config: functionConfig } = manifest
   expect(bundles.length).toBe(1)
   expect(bundles[0].format).toBe('eszip2')
   expect(generatedFiles.includes(bundles[0].asset)).toBe(true)
-  expect(routes[0].generator).toBeUndefined()
-  expect(routes[1].name).toBe('Function two')
-  expect(routes[1].generator).toBe('@netlify/fake-plugin@1.0.0')
-  expect(routes[2].generator).toBe('internalFunc')
 
   // respects excludedPath from deploy config
-  expect(functionConfig.func2).toEqual({ excluded_patterns: ['^/func2/skip/?$'] })
+  expect(functionConfig.func2).toEqual({
+    excluded_patterns: ['^/func2/skip/?$'],
+    name: 'Function two',
+    generator: '@netlify/fake-plugin@1.0.0',
+  })
+
+  // respects in-source config
+  expect(functionConfig.func3).toEqual({
+    name: 'in-config-function',
+    on_error: 'bypass',
+    generator: 'internalFunc',
+  })
 
   await cleanup()
 })

diff --git a/node/bundler.ts b/node/bundler.ts
@@ -2,17 +2,15 @@ import { promises as fs } from 'fs'
 import { join } from 'path'
 
 import commonPathPrefix from 'common-path-prefix'
-import isPathInside from 'is-path-inside'
 import { v4 as uuidv4 } from 'uuid'
 
 import { importMapSpecifier } from '../shared/consts.js'
 
 import { DenoBridge, DenoOptions, OnAfterDownloadHook, OnBeforeDownloadHook } from './bridge.js'
 import type { Bundle } from './bundle.js'
-import { getFunctionConfig } from './config.js'
+import { FunctionConfig, getFunctionConfig } from './config.js'
 import { Declaration, mergeDeclarations } from './declaration.js'
 import { load as loadDeployConfig } from './deploy_config.js'
-import { EdgeFunction } from './edge_function.js'
 import { FeatureFlags, getFlags } from './feature_flags.js'
 import { findFunctions } from './finder.js'
 import { bundle as bundleESZIP } from './formats/eszip.js'
@@ -128,18 +126,17 @@ const bundle = async (
 
   // Creating a final declarations array by combining the TOML file with the
   // deploy configuration API and the in-source configuration.
-  const declarationsFromConfig = mergeDeclarations(
+  const declarations = mergeDeclarations(
     tomlDeclarations,
     userFunctionsWithConfig,
     internalFunctionsWithConfig,
     deployConfig.declarations,
   )
 
-  // If any declarations are autogenerated and are missing the generator field
-  // add a default string.
-  const declarations = internalSrcFolder
-    ? declarationsFromConfig.map((declaration) => addGeneratorFieldIfMissing(declaration, functions, internalSrcFolder))
-    : declarationsFromConfig
+  const internalFunctionConfig = createFunctionConfig({
+    internalFunctionsWithConfig,
+    declarations,
+  })
 
   const manifest = await writeManifest({
     bundles: [functionBundle],
@@ -148,7 +145,7 @@ const bundle = async (
     featureFlags,
     functions,
     userFunctionConfig: userFunctionsWithConfig,
-    internalFunctionConfig: internalFunctionsWithConfig,
+    internalFunctionConfig,
     importMap: importMapSpecifier,
     layers: deployConfig.layers,
   })
@@ -186,19 +183,44 @@ const getBasePath = (sourceDirectories: string[], inputBasePath?: string) => {
   return commonPathPrefix(sourceDirectories)
 }
 
-export const addGeneratorFieldIfMissing = (
-  declaration: Declaration,
-  functions: EdgeFunction[],
-  internalFunctionsPath?: string,
-) => {
-  const fullFuncPath = functions?.find((func) => func.name === declaration.function)?.path
+interface MergeWithDeclarationConfigOptions {
+  functionName: string
+  config: FunctionConfig
+  declarations: Declaration[]
+}
 
-  // If function path is in the internalFunctionsPath, we assume it is autogenerated.
-  const isInternal = Boolean(internalFunctionsPath && fullFuncPath && isPathInside(fullFuncPath, internalFunctionsPath))
+// We used to allow the `name` and `generator` fields to be defined at the
+// declaration level. We want these properties to live at the function level
+// in their config object, so we translate that for backwards-compatibility.
+const mergeWithDeclarationConfig = ({ functionName, config, declarations }: MergeWithDeclarationConfigOptions) => {
+  const declaration = declarations?.find((decl) => decl.function === functionName)
 
-  const generatorFallback = isInternal ? 'internalFunc' : undefined
-  return { ...declaration, generator: declaration.generator || generatorFallback }
+  return {
+    ...config,
+    name: declaration?.name || config.name,
+    generator: declaration?.generator || config.generator,
+  }
 }
 
+const addGeneratorFallback = (config: FunctionConfig) => ({
+  ...config,
+  generator: config.generator || 'internalFunc',
+})
+
+interface CreateFunctionConfigOptions {
+  internalFunctionsWithConfig: Record<string, FunctionConfig>
+  declarations: Declaration[]
+}
+
+const createFunctionConfig = ({ internalFunctionsWithConfig, declarations }: CreateFunctionConfigOptions) =>
+  Object.entries(internalFunctionsWithConfig).reduce((acc, [functionName, config]) => {
+    const mergedConfigFields = mergeWithDeclarationConfig({ functionName, config, declarations })
+
+    return {
+      ...acc,
+      [functionName]: addGeneratorFallback(mergedConfigFields),
+    }
+  }, {} as Record<string, FunctionConfig>)
+
 export { bundle }
 export type { BundleOptions }
diff --git a/node/config.test.ts b/node/config.test.ts
@@ -127,6 +127,25 @@ const functions = [
         export const config = { path: "/home" }
       `,
   },
+  {
+    testName: 'config with path, generator, name and onError`',
+    expectedConfig: {
+      path: '/home',
+      generator: '@netlify/fake-plugin@1.0.0',
+      name: 'a displayName',
+      onError: 'bypass',
+    },
+    name: 'func6',
+    source: `
+        export default async () => new Response("Hello from function three")
+
+        export const config = { path: "/home",
+          generator: '@netlify/fake-plugin@1.0.0',
+          name: 'a displayName',
+          onError: 'bypass',
+        }
+      `,
+  },
 ]
 describe('`getFunctionConfig` extracts configuration properties from function file', () => {
   test.each(functions)('$testName', async (func) => {

diff --git a/node/config.ts b/node/config.ts
@@ -29,7 +29,7 @@ export const enum Cache {
 
 export type Path = `/${string}`
 
-export type OnError = 'fail' | 'bypass' | `/${string}`
+export type OnError = 'fail' | 'bypass' | Path
 
 export const isValidOnError = (value: unknown): value is OnError => {
   if (typeof value === 'undefined') return true
@@ -42,6 +42,8 @@ export interface FunctionConfig {
   path?: Path | Path[]
   excludedPath?: Path | Path[]
   onError?: OnError
+  name?: string
+  generator?: string
 }
 
 const getConfigExtractor = () => {

diff --git a/node/declaration.ts b/node/declaration.ts
@@ -5,6 +5,7 @@ import { FunctionConfig, Path } from './config.js'
 interface BaseDeclaration {
   cache?: string
   function: string
+  // todo: remove these two after a while and only support in-source config for non-route related configs
   name?: string
   generator?: string
 }

diff --git a/node/manifest.test.ts b/node/manifest.test.ts
@@ -3,7 +3,7 @@ import { env } from 'process'
 import { test, expect, vi } from 'vitest'
 
 import { BundleFormat } from './bundle.js'
-import { FunctionConfig } from './config.js'
+import { Cache, FunctionConfig } from './config.js'
 import { Declaration } from './declaration.js'
 import { generateManifest } from './manifest.js'
 
@@ -34,48 +34,39 @@ test('Generates a manifest with different bundles', () => {
 })
 
 test('Generates a manifest with display names', () => {
-  const functions = [
-    { name: 'func-1', path: '/path/to/func-1.ts' },
-    { name: 'func-2', path: '/path/to/func-2.ts' },
-  ]
-  const declarations: Declaration[] = [
-    { function: 'func-1', name: 'Display Name', path: '/f1/*' },
-    { function: 'func-2', path: '/f2/*' },
-  ]
-  const manifest = generateManifest({ bundles: [], declarations, functions })
+  const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
+  const declarations: Declaration[] = [{ function: 'func-1', path: '/f1/*' }]
 
-  const expectedRoutes = [
-    { function: 'func-1', name: 'Display Name', pattern: '^/f1/.*/?$' },
-    { function: 'func-2', pattern: '^/f2/.*/?$' },
-  ]
+  const internalFunctionConfig: Record<string, FunctionConfig> = {
+    'func-1': {
+      name: 'Display Name',
+    },
+  }
+  const manifest = generateManifest({ bundles: [], declarations, functions, internalFunctionConfig })
 
+  const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
+  expect(manifest.function_config).toEqual({
+    'func-1': { name: 'Display Name' },
+  })
   expect(manifest.routes).toEqual(expectedRoutes)
   expect(manifest.bundler_version).toBe(env.npm_package_version as string)
 })
 
 test('Generates a manifest with a generator field', () => {
-  const functions = [
-    { name: 'func-1', path: '/path/to/func-1.ts' },
-    { name: 'func-2', path: '/path/to/func-2.ts' },
-    { name: 'func-3', path: '/path/to/func-3.ts' },
-  ]
-
-  const declarations: Declaration[] = [
-    { function: 'func-1', generator: '@netlify/fake-plugin@1.0.0', path: '/f1/*' },
-    { function: 'func-2', path: '/f2/*' },
-    { function: 'func-3', generator: '@netlify/fake-plugin@1.0.0', cache: 'manual', path: '/f3' },
-  ]
-  const manifest = generateManifest({ bundles: [], declarations, functions })
+  const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
 
-  const expectedRoutes = [
-    { function: 'func-1', generator: '@netlify/fake-plugin@1.0.0', pattern: '^/f1/.*/?$' },
-    { function: 'func-2', pattern: '^/f2/.*/?$' },
-  ]
-  const expectedPostCacheRoutes = [{ function: 'func-3', generator: '@netlify/fake-plugin@1.0.0', pattern: '^/f3/?$' }]
+  const declarations: Declaration[] = [{ function: 'func-1', path: '/f1/*' }]
+  const internalFunctionConfig: Record<string, FunctionConfig> = {
+    'func-1': {
+      generator: '@netlify/fake-plugin@1.0.0',
+    },
+  }
+  const manifest = generateManifest({ bundles: [], declarations, functions, internalFunctionConfig })
 
+  const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
+  const expectedFunctionConfig = { 'func-1': { generator: '@netlify/fake-plugin@1.0.0' } }
   expect(manifest.routes).toEqual(expectedRoutes)
-  expect(manifest.post_cache_routes).toEqual(expectedPostCacheRoutes)
-  expect(manifest.bundler_version).toBe(env.npm_package_version as string)
+  expect(manifest.function_config).toEqual(expectedFunctionConfig)
 })
 
 test('Generates a manifest with excluded paths and patterns', () => {
@@ -84,13 +75,13 @@ test('Generates a manifest with excluded paths and patterns', () => {
     { name: 'func-2', path: '/path/to/func-2.ts' },
   ]
   const declarations: Declaration[] = [
-    { function: 'func-1', name: 'Display Name', path: '/f1/*', excludedPath: '/f1/exclude' },
+    { function: 'func-1', path: '/f1/*', excludedPath: '/f1/exclude' },
     { function: 'func-2', pattern: '^/f2/.*/?$', excludedPattern: '^/f2/exclude$' },
   ]
   const manifest = generateManifest({ bundles: [], declarations, functions })
 
   const expectedRoutes = [
-    { function: 'func-1', name: 'Display Name', pattern: '^/f1/.*/?$' },
+    { function: 'func-1', pattern: '^/f1/.*/?$' },
     { function: 'func-2', pattern: '^/f2/.*/?$' },
   ]
 
@@ -102,13 +93,64 @@ test('Generates a manifest with excluded paths and patterns', () => {
   expect(manifest.bundler_version).toBe(env.npm_package_version as string)
 })
 
+test('Filters out internal in-source configurations in user created functions', () => {
+  const functions = [
+    { name: 'func-1', path: '/path/to/func-1.ts' },
+    { name: 'func-2', path: '/path/to/func-2.ts' },
+  ]
+  const declarations: Declaration[] = [
+    { function: 'func-1', path: '/f1/*' },
+    { function: 'func-2', pattern: '^/f2/.*/?$' },
+  ]
+  const userFunctionConfig: Record<string, FunctionConfig> = {
+    'func-1': {
+      onError: '/custom-error',
+      cache: Cache.Manual,
+      excludedPath: '/f1/exclude',
+      path: '/path/to/func-1.ts',
+      name: 'User function',
+      generator: 'fake-generator',
+    },
+  }
+  const internalFunctionConfig: Record<string, FunctionConfig> = {
+    'func-2': {
+      onError: 'bypass',
+      cache: Cache.Off,
+      excludedPath: '/f2/exclude',
+      path: '/path/to/func-2.ts',
+      name: 'Internal function',
+      generator: 'internal-generator',
+    },
+  }
+  const manifest = generateManifest({
+    bundles: [],
+    declarations,
+    functions,
+    userFunctionConfig,
+    internalFunctionConfig,
+  })
+  expect(manifest.function_config).toEqual({
+    'func-1': {
+      on_error: '/custom-error',
+      excluded_patterns: ['^/f1/exclude/?$'],
+    },
+    'func-2': {
+      on_error: 'bypass',
+      cache: Cache.Off,
+      name: 'Internal function',
+      generator: 'internal-generator',
+      excluded_patterns: ['^/f2/exclude/?$'],
+    },
+  })
+})
+
 test('Includes failure modes in manifest', () => {
   const functions = [
     { name: 'func-1', path: '/path/to/func-1.ts' },
     { name: 'func-2', path: '/path/to/func-2.ts' },
   ]
   const declarations: Declaration[] = [
-    { function: 'func-1', name: 'Display Name', path: '/f1/*' },
+    { function: 'func-1', path: '/f1/*' },
     { function: 'func-2', pattern: '^/f2/.*/?$' },
   ]
   const userFunctionConfig: Record<string, FunctionConfig> = {
@@ -118,7 +160,7 @@ test('Includes failure modes in manifest', () => {
   }
   const manifest = generateManifest({ bundles: [], declarations, functions, userFunctionConfig })
   expect(manifest.function_config).toEqual({
-    'func-1': { excluded_patterns: [], on_error: '/custom-error' },
+    'func-1': { on_error: '/custom-error' },
   })
 })