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: propagate onError config property to manifest #328

Merged
merged 12 commits into from
Mar 8, 2023
18 changes: 18 additions & 0 deletions node/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,24 @@ const functions = [
edge_functions_invalid_config_throw: false,
},
},
{
testName: 'config with correct onError',
expectedConfig: { onError: 'bypass' },
name: 'func5',
source: `
export default async () => new Response("Hello from function two")
export const config = { onError: "bypass" }
`,
},
{
testName: 'config with wrong onError',
name: 'func7',
source: `
export default async () => new Response("Hello from function two")
export const config = { onError: "foo" }
`,
error: /The 'onError' configuration property in edge function at .*/,
},
{
testName: 'config with `path`',
expectedConfig: { path: '/home' },
Expand Down
28 changes: 23 additions & 5 deletions node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,19 @@ export const enum Cache {
Manual = 'manual',
}

export type OnError = 'fail' | 'bypass' | `/${string}`

export const isValidOnError = (value: unknown): value is OnError => {
if (typeof value === 'undefined') return true
Copy link
Member

Choose a reason for hiding this comment

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

Curious why you're not just checking value === undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

age-old insecurities about how JS handles null vs undefined. blame brendan eich

if (typeof value !== 'string') return false
return value === 'fail' || value === 'bypass' || value.startsWith('/')
}

export interface FunctionConfig {
cache?: Cache
path?: string | string[]
excludedPath?: string | string[]
onError?: OnError
}

const getConfigExtractor = () => {
Expand Down Expand Up @@ -89,17 +98,26 @@ export const getFunctionConfig = async (
log.user(stdout)
}

try {
const collectorData = await fs.readFile(collector.path, 'utf8')
let collectorData: FunctionConfig = {}

return JSON.parse(collectorData) as FunctionConfig
try {
const collectorDataJSON = await fs.readFile(collector.path, 'utf8')
collectorData = JSON.parse(collectorDataJSON) as FunctionConfig
} catch {
handleConfigError(func, ConfigExitCode.UnhandledError, stderr, log, featureFlags)

return {}
} finally {
await collector.cleanup()
}

if (!isValidOnError(collectorData.onError)) {
throw new BundleError(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this is new behaviour, I think it's fine to throw on error - no need to use the flag.

new Error(
`The 'onError' configuration property in edge function at '${func.path}' must be one of 'fail', 'bypass', or a path starting with '/'. Got '${collectorData.onError}'. More on the Edge Functions API at https://ntl.fyi/edge-api.`,
),
)
}

return collectorData
}

const handleConfigError = (
Expand Down
21 changes: 21 additions & 0 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { env } from 'process'
import { test, expect, vi } from 'vitest'

import { BundleFormat } from './bundle.js'
import { FunctionConfig } from './config.js'
import { Declaration } from './declaration.js'
import { generateManifest } from './manifest.js'

Expand Down Expand Up @@ -101,6 +102,26 @@ test('Generates a manifest with excluded paths and patterns', () => {
expect(manifest.bundler_version).toBe(env.npm_package_version as string)
})

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-2', pattern: '^/f2/.*/?$' },
]
const functionConfig: Record<string, FunctionConfig> = {
'func-1': {
onError: '/custom-error',
},
}
const manifest = generateManifest({ bundles: [], declarations, functions, functionConfig })
expect(manifest.function_config).toEqual({
'func-1': { excluded_patterns: [], on_error: '/custom-error' },
})
})

test('Excludes functions for which there are function files but no matching config declarations', () => {
const bundle1 = {
extension: '.ext2',
Expand Down
16 changes: 7 additions & 9 deletions node/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ interface Route {
}
interface EdgeFunctionConfig {
excluded_patterns: string[]
on_error?: string
}
interface Manifest {
bundler_version: string
Expand All @@ -41,13 +42,6 @@ interface GenerateManifestOptions {
layers?: Layer[]
}

interface Route {
function: string
name?: string
pattern: string
generator?: string
}

// JavaScript regular expressions are converted to strings with leading and
// trailing slashes, so any slashes inside the expression itself are escaped
// as `//`. This function deserializes that back into a single slash, which
Expand All @@ -58,7 +52,7 @@ const sanitizeEdgeFunctionConfig = (config: Record<string, EdgeFunctionConfig>):
const newConfig: Record<string, EdgeFunctionConfig> = {}

for (const [name, functionConfig] of Object.entries(config)) {
if (functionConfig.excluded_patterns.length !== 0) {
if (functionConfig.excluded_patterns.length !== 0 || functionConfig.on_error) {
newConfig[name] = functionConfig
}
}
Expand All @@ -81,13 +75,17 @@ const generateManifest = ({
functions.map(({ name }) => [name, { excluded_patterns: [] }]),
)

for (const [name, { excludedPath }] of Object.entries(functionConfig)) {
for (const [name, { excludedPath, onError }] of Object.entries(functionConfig)) {
if (excludedPath) {
const paths = Array.isArray(excludedPath) ? excludedPath : [excludedPath]
const excludedPatterns = paths.map(pathToRegularExpression).map(serializePattern)

manifestFunctionConfig[name].excluded_patterns.push(...excludedPatterns)
}

if (onError) {
manifestFunctionConfig[name].on_error = onError
}
}

declarations.forEach((declaration) => {
Expand Down
1 change: 1 addition & 0 deletions node/validation/manifest/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const functionConfigSchema = {
'excluded_patterns needs to be an array of regex that starts with ^ and ends with $ without any additional slashes before and afterwards',
},
},
on_error: { type: 'string' },
},
}

Expand Down