Skip to content
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

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

Merged
merged 9 commits into from
Mar 24, 2023
22 changes: 14 additions & 8 deletions node/bundler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [
{
Expand All @@ -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/[email protected]')
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/[email protected]',
})

// respects in-source config
expect(functionConfig.func3).toEqual({
name: 'in-config-function',
on_error: 'bypass',
generator: 'internalFunc',
})

await cleanup()
})
Expand Down
60 changes: 40 additions & 20 deletions node/bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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],
Expand All @@ -148,7 +145,7 @@ const bundle = async (
featureFlags,
functions,
userFunctionConfig: userFunctionsWithConfig,
internalFunctionConfig: internalFunctionsWithConfig,
internalFunctionConfig,
importMap: importMapSpecifier,
layers: deployConfig.layers,
})
Expand Down Expand Up @@ -186,19 +183,42 @@ 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 {
name: 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))
// Merged any old-style non-in-source configs from declaration.
const mergeWithDeclarationConfig = ({ name, config, declarations }: MergeWithDeclarationConfigOptions) => {
const declaration = declarations?.find((decl) => decl.function === name)

const generatorFallback = isInternal ? 'internalFunc' : undefined
return { ...declaration, generator: declaration.generator || generatorFallback }
return {
...config,
name: declaration?.name || config.name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This slightly confused me, until i figured that this is the displayName and the passed in name is the functionName. Could we make this more clear? maybe alias the input name as functionName or add a comment here that this is the display name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's not the case, the name field is the displayname in all cases with Edge Functions

Copy link
Contributor

@danez danez Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we could do

Suggested change
name: declaration?.name || config.name,
name,

here? I don't believe so. With input I mean the name variable on Line 193

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh yeah that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the way I named it now

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) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I find this function a bit hard to follow. The name isn't super descriptive — it says it creates function configs but it's iterating one of the parameters and calling two other functions inside. I feel like an abstraction like addGeneratorFallback, which is simply adding a field, is adding to the confusion more than the benefits it brings.

Object.entries(internalFunctionsWithConfig).reduce((acc, [name, config]) => {
const mergedConfigFields = mergeWithDeclarationConfig({ name, config, declarations })

return {
...acc,
[name]: addGeneratorFallback(mergedConfigFields),
}
}, {} as Record<string, FunctionConfig>)

export { bundle }
export type { BundleOptions }
19 changes: 19 additions & 0 deletions node/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,25 @@ const functions = [
export const config = { path: "/home" }
`,
},
{
testName: 'config with path, generator, name and onError`',
expectedConfig: {
path: '/home',
generator: '@netlify/[email protected]',
name: 'a displayName',
onError: 'bypass',
},
name: 'func6',
source: `
export default async () => new Response("Hello from function three")

export const config = { path: "/home",
generator: '@netlify/[email protected]',
name: 'a displayName',
onError: 'bypass',
}
`,
},
]
describe('`getFunctionConfig` extracts configuration properties from function file', () => {
test.each(functions)('$testName', async (func) => {
Expand Down
4 changes: 3 additions & 1 deletion node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -42,6 +42,8 @@ export interface FunctionConfig {
path?: Path | Path[]
excludedPath?: Path | Path[]
onError?: OnError
name?: string
generator?: string
}

const getConfigExtractor = () => {
Expand Down
1 change: 1 addition & 0 deletions node/declaration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's "after a while"? Should this TODO be an issue instead?

name?: string
generator?: string
}
Expand Down
116 changes: 79 additions & 37 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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/[email protected]', path: '/f1/*' },
{ function: 'func-2', path: '/f2/*' },
{ function: 'func-3', generator: '@netlify/[email protected]', 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/[email protected]', pattern: '^/f1/.*/?$' },
{ function: 'func-2', pattern: '^/f2/.*/?$' },
]
const expectedPostCacheRoutes = [{ function: 'func-3', generator: '@netlify/[email protected]', pattern: '^/f3/?$' }]
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1/*' }]
const internalFunctionConfig: Record<string, FunctionConfig> = {
'func-1': {
generator: '@netlify/[email protected]',
},
}
const manifest = generateManifest({ bundles: [], declarations, functions, internalFunctionConfig })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
const expectedFunctionConfig = { 'func-1': { generator: '@netlify/[email protected]' } }
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', () => {
Expand All @@ -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/.*/?$' },
]

Expand All @@ -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> = {
Expand All @@ -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' },
})
})

Expand Down
Loading