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 excluded_patterns into function_config #274

Merged
merged 8 commits into from
Jan 6, 2023
4 changes: 2 additions & 2 deletions node/bundler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,14 @@ 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 { bundles, routes } = 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)

// respects excludedPath from deploy config
expect(routes[1].excluded_pattern).toEqual('^/func2/skip/?$')
expect(functionConfig.func2).toEqual({ excluded_patterns: ['^/func2/skip/?$'] })

await cleanup()
})
Expand Down
1 change: 1 addition & 0 deletions node/bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ const bundle = async (
declarations,
distDirectory,
functions,
functionConfig: functionsWithConfig,
importMap: importMapSpecifier,
layers: deployConfig.layers,
})
Expand Down
14 changes: 10 additions & 4 deletions node/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ test('Ignores function paths from the in-source `config` function if the feature
})
const generatedFiles = await fs.readdir(distPath)

expect(result.functions.length).toBe(6)
expect(result.functions.length).toBe(7)
expect(generatedFiles.length).toBe(2)

const manifestFile = await fs.readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -165,27 +165,33 @@ test('Loads function paths from the in-source `config` function', async () => {
})
const generatedFiles = await fs.readdir(distPath)

expect(result.functions.length).toBe(6)
expect(result.functions.length).toBe(7)
expect(generatedFiles.length).toBe(2)

const manifestFile = await fs.readFile(resolve(distPath, 'manifest.json'), 'utf8')
const manifest = JSON.parse(manifestFile)
const { bundles, routes, post_cache_routes: postCacheRoutes } = manifest
const { bundles, routes, post_cache_routes: postCacheRoutes, 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.length).toBe(5)
expect(routes.length).toBe(6)
expect(routes[0]).toEqual({ function: 'framework-func2', pattern: '^/framework-func2/?$' })
expect(routes[1]).toEqual({ function: 'user-func2', pattern: '^/user-func2/?$' })
expect(routes[2]).toEqual({ function: 'framework-func1', pattern: '^/framework-func1/?$' })
expect(routes[3]).toEqual({ function: 'user-func1', pattern: '^/user-func1/?$' })
expect(routes[4]).toEqual({ function: 'user-func3', pattern: '^/user-func3/?$' })
expect(routes[5]).toEqual({ function: 'user-func5', pattern: '^/user-func5/.*/?$' })

expect(postCacheRoutes.length).toBe(1)
expect(postCacheRoutes[0]).toEqual({ function: 'user-func4', pattern: '^/user-func4/?$' })

expect(Object.keys(functionConfig)).toHaveLength(1)
expect(functionConfig['user-func5']).toEqual({
excluded_patterns: ['^/user-func5/excluded/?$'],
})

await cleanup()
})

Expand Down
1 change: 1 addition & 0 deletions node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const enum Cache {
export interface FunctionConfig {
cache?: Cache
path?: string | string[]
excludedPath?: string | string[]
}

const getConfigExtractor = () => {
Expand Down
9 changes: 4 additions & 5 deletions node/declaration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ export const getDeclarationsFromConfig = (
const paths = Array.isArray(config.path) ? config.path : [config.path]

paths.forEach((path) => {
declarations.push({ ...declaration, ...config, path })
declarations.push({ ...declaration, cache: config.cache, path })
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this bit. Why are we suddenly setting the cache property specifically rather than all of config? And does it mean that if we add more configuration properties to ISC, will we have to add them here in addition to config.ts?

Copy link
Contributor Author

@Skn0tt Skn0tt Jan 5, 2023

Choose a reason for hiding this comment

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

This is necessary because config now not only contains cache and path, but also excludedPatterns, which should not be part of the declaration. This means that we'll have to manually add all declaration-specific ISC properties here, yes.

Now that I think about it, should preCache / postCache be part of functions config as well? Are there cases where the same edge function sometimes should run before, and sometimes after cache? (this is out of scope for this PR)

Copy link
Member

Choose a reason for hiding this comment

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

This is necessary because config now not only contains cache and path, but also excludedPatterns, which should not be part of the declaration.

I guess you could push to the declaration everything except excludedPatterns, which you know shouldn't be there. If we add more properties to ISC we wouldn't need to manually include them here?

Happy to defer to you though.

})

// With an in-source config without a path, add the config to the declaration
} else {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { path, ...rest } = config
const { path, excludedPath, ...rest } = config
declarations.push({ ...declaration, ...rest })
}

Expand All @@ -60,15 +60,14 @@ export const getDeclarationsFromConfig = (
// Finally, we must create declarations for functions that are not declared
// in the TOML at all.
for (const name in functionsConfig) {
const { ...config } = functionsConfig[name]
const { path } = functionsConfig[name]
const { cache, path } = functionsConfig[name]

// If we have path specified create a declaration for each path
if (!functionsVisited.has(name) && path) {
const paths = Array.isArray(path) ? path : [path]

paths.forEach((singlePath) => {
declarations.push({ ...config, function: name, path: singlePath })
declarations.push({ cache, function: name, path: singlePath })
})
}
}
Expand Down
8 changes: 6 additions & 2 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,15 @@ test('Generates a manifest with excluded paths and patterns', () => {
const manifest = generateManifest({ bundles: [], declarations, functions })

const expectedRoutes = [
{ function: 'func-1', name: 'Display Name', pattern: '^/f1/.*/?$', excluded_pattern: '^/f1/exclude/?$' },
{ function: 'func-2', pattern: '^/f2/.*/?$', excluded_pattern: '^/f2/exclude$' },
{ function: 'func-1', name: 'Display Name', pattern: '^/f1/.*/?$' },
{ function: 'func-2', pattern: '^/f2/.*/?$' },
]

expect(manifest.routes).toEqual(expectedRoutes)
expect(manifest.function_config).toEqual({
'func-1': { excluded_patterns: ['^/f1/exclude/?$'] },
'func-2': { excluded_patterns: ['^/f2/exclude$'] },
})
expect(manifest.bundler_version).toBe(env.npm_package_version as string)
})

Expand Down
70 changes: 44 additions & 26 deletions node/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,21 @@ import { join } from 'path'
import globToRegExp from 'glob-to-regexp'

import type { Bundle } from './bundle.js'
import { Cache } from './config.js'
import { Cache, FunctionConfig } from './config.js'
import type { Declaration } from './declaration.js'
import { EdgeFunction } from './edge_function.js'
import { Layer } from './layer.js'
import { getPackageVersion } from './package_json.js'
import { nonNullable } from './utils/non_nullable.js'

interface GenerateManifestOptions {
bundles?: Bundle[]
declarations?: Declaration[]
functions: EdgeFunction[]
importMap?: string
layers?: Layer[]
}

/* eslint-disable camelcase */
interface Route {
function: string
name?: string
pattern: string
excluded_pattern?: string
}
interface EdgeFunctionConfig {
excluded_patterns: string[]
}
interface Manifest {
bundler_version: string
Expand All @@ -33,9 +27,20 @@ interface Manifest {
layers: { name: string; flag: string }[]
routes: Route[]
post_cache_routes: Route[]
function_config: Record<string, EdgeFunctionConfig>
}

/* eslint-enable camelcase */

interface GenerateManifestOptions {
bundles?: Bundle[]
declarations?: Declaration[]
functions: EdgeFunction[]
functionConfig?: Record<string, FunctionConfig>
importMap?: string
layers?: Layer[]
}

interface Route {
function: string
name?: string
Expand All @@ -44,15 +49,39 @@ interface Route {

const serializePattern = (regex: RegExp) => regex.source.replace(/\\\//g, '/')

const sanitizeEdgeFunctionConfig = (config: Record<string, EdgeFunctionConfig>): Record<string, EdgeFunctionConfig> => {
const newConfig: Record<string, EdgeFunctionConfig> = {}

for (const [name, functionConfig] of Object.entries(config)) {
if (functionConfig.excluded_patterns.length !== 0) {
newConfig[name] = functionConfig
}
}

return newConfig
}

const generateManifest = ({
bundles = [],
declarations = [],
functions,
functionConfig = {},
importMap,
layers = [],
}: GenerateManifestOptions) => {
const preCacheRoutes: Route[] = []
const postCacheRoutes: Route[] = []
const manifestFunctionConfig: Manifest['function_config'] = Object.fromEntries(
Copy link
Member

Choose a reason for hiding this comment

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

I think this means that there will be an entry in function_config for every function, even if the object is empty. Is that intended? Could we simply omit a function from the object if it doesn't have any configuration properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought the same when I wrote this first. Then I replaced it with 7c49753, found that to be too much indirection, and went back. Committed it again, let me know what you think ^^

functions.map(({ name }) => [name, { excluded_patterns: [] }]),
)

for (const [name, { excludedPath }] 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)
}
}

declarations.forEach((declaration) => {
const func = functions.find(({ name }) => declaration.function === name)
Expand All @@ -69,7 +98,7 @@ const generateManifest = ({
}
const excludedPattern = getExcludedRegularExpression(declaration)
if (excludedPattern) {
route.excluded_pattern = serializePattern(excludedPattern)
manifestFunctionConfig[func.name].excluded_patterns.push(serializePattern(excludedPattern))
}

if (declaration.cache === Cache.Manual) {
Expand All @@ -89,6 +118,7 @@ const generateManifest = ({
bundler_version: getPackageVersion(),
layers,
import_map: importMap,
function_config: sanitizeEdgeFunctionConfig(manifestFunctionConfig),
}

return manifest
Expand Down Expand Up @@ -125,24 +155,12 @@ const getExcludedRegularExpression = (declaration: Declaration) => {
}
}

interface WriteManifestOptions {
bundles: Bundle[]
declarations: Declaration[]
interface WriteManifestOptions extends GenerateManifestOptions {
distDirectory: string
functions: EdgeFunction[]
importMap?: string
layers?: Layer[]
}

const writeManifest = async ({
bundles,
declarations = [],
distDirectory,
functions,
importMap,
layers,
}: WriteManifestOptions) => {
const manifest = generateManifest({ bundles, declarations, functions, importMap, layers })
const writeManifest = async ({ distDirectory, ...rest }: WriteManifestOptions) => {
const manifest = generateManifest(rest)
const manifestPath = join(distDirectory, 'manifest.json')

await fs.writeFile(manifestPath, JSON.stringify(manifest))
Expand Down
17 changes: 17 additions & 0 deletions node/validation/manifest/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@ const routesSchema = {
additionalProperties: false,
}

const functionConfigSchema = {
type: 'object',
required: [],
properties: {
excluded_patterns: {
type: 'array',
items: {
type: 'string',
format: 'regexPattern',
errorMessage:
'excluded_patterns needs to be an array of regex that starts with ^ and ends with $ without any additional slashes before and afterwards',
},
},
},
}

const layersSchema = {
type: 'object',
required: ['flag', 'name'],
Expand Down Expand Up @@ -57,6 +73,7 @@ const edgeManifestSchema = {
},
import_map: { type: 'string' },
bundler_version: { type: 'string' },
function_config: { type: 'object', items: functionConfigSchema },
},
additionalProperties: false,
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export default async () => new Response('Hello from user function 5.')

export const config = {
path: '/user-func5/*',
excludedPath: '/user-func5/excluded',
}