From 3379f40ff8657cdbf03cc9be7cc08f2dc4e1ce49 Mon Sep 17 00:00:00 2001 From: JGAntunes Date: Thu, 28 Dec 2023 17:19:28 +0000 Subject: [PATCH 01/10] fix(dev): reloading edge functions for child dependencies --- src/lib/edge-functions/registry.ts | 82 +++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 3144a49117a..700f4742c37 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -23,6 +23,8 @@ type EdgeFunctionEvent = 'buildError' | 'loaded' | 'reloaded' | 'reloading' | 'r type Route = Omit & { pattern: RegExp } type RunIsolate = Awaited> +type ModuleJson = ModuleGraph['modules'][number] + const featureFlags = { edge_functions_correct_order: true } interface EdgeFunctionsRegistryOptions { @@ -40,6 +42,40 @@ interface EdgeFunctionsRegistryOptions { servePath: string } +/** + * Helper method which, given a edge bundler graph module and an index of modules by path, traverses its dependency tree + * and returns an array of all of ist local dependencies + */ +function traverseLocalDependencies( + { dependencies = [] }: ModuleJson, + modulesByPath: Map, +): string[] { + return dependencies + .map((dependency) => { + // We're interested in tracking local dependencies, so we only look at + // specifiers with the `file:` protocol. + if ( + dependency.code === undefined || + typeof dependency.code.specifier !== 'string' || + !dependency.code.specifier.startsWith('file://') + ) { + return [] + } + const { specifier: dependencyURL } = dependency.code + const dependencyPath = fileURLToPath(dependencyURL) + const dependencyModule = modulesByPath.get(dependencyPath) + + // No module indexed for this dependency + if (dependencyModule === undefined) { + return [dependencyPath] + } + + // Keep traversing the child dependencies and return the current dependency path + return [...traverseLocalDependencies(dependencyModule, modulesByPath), dependencyPath] + }) + .reduce((acc, deps) => [...acc, ...deps], []) +} + export class EdgeFunctionsRegistry { private buildError: Error | null = null private bundler: typeof import('@netlify/edge-bundler') @@ -420,36 +456,44 @@ export class EdgeFunctionsRegistry { // Mapping file URLs to names of functions that use them as dependencies. const dependencyPaths = new Map() + // Map modules by path, acting as an helper index + const modulesByPath = new Map() + + // Map function modules, acting as an helper index + const functionModules = new Map() + const { modules } = graph - modules.forEach(({ dependencies = [], specifier }) => { + modules.forEach((module) => { + // We're interested in tracking local dependencies, so we only look at + // specifiers with the `file:` protocol. + const { specifier } = module if (!specifier.startsWith('file://')) { return } + // Populate the module index const path = fileURLToPath(specifier) - const functionMatch = functionPaths.get(path) + modulesByPath.set(path, module) - if (!functionMatch) { - return + // Populate the function modules index + const functionMatch = functionPaths.get(path) + if (functionMatch) { + functionModules.set(path, module) } + }) - dependencies.forEach((dependency) => { - // We're interested in tracking local dependencies, so we only look at - // specifiers with the `file:` protocol. - if ( - dependency.code === undefined || - typeof dependency.code.specifier !== 'string' || - !dependency.code.specifier.startsWith('file://') - ) { - return - } - - const { specifier: dependencyURL } = dependency.code - const dependencyPath = fileURLToPath(dependencyURL) + // We start from our functions and we traverse through their dependency tree + functionModules.forEach((functionModule) => { + const { specifier } = functionModule + const traversedPaths = traverseLocalDependencies(functionModule, modulesByPath) + const path = fileURLToPath(specifier) + const functionName = functionPaths.get(path) + if (!functionName) return + // With the paths for the local dependencies we build the dependency path map + traversedPaths.forEach((dependencyPath) => { const functions = dependencyPaths.get(dependencyPath) || [] - - dependencyPaths.set(dependencyPath, [...functions, functionMatch]) + dependencyPaths.set(dependencyPath, [...functions, functionName]) }) }) From d980799e72ea9bc497139ac1d4b8fe5b3b129fd6 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 17:26:39 +0100 Subject: [PATCH 02/10] chore: add test --- .../commands/dev/edge-functions.test.ts | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/integration/commands/dev/edge-functions.test.ts b/tests/integration/commands/dev/edge-functions.test.ts index 63f6b83cf3a..fe1403811ce 100644 --- a/tests/integration/commands/dev/edge-functions.test.ts +++ b/tests/integration/commands/dev/edge-functions.test.ts @@ -1,11 +1,13 @@ import process from 'process' import execa from 'execa' +import fetch from 'node-fetch' import { describe, expect, expectTypeOf, test } from 'vitest' +import { withDevServer } from '../../utils/dev-server.js' import { FixtureTestContext, setupFixtureTests } from '../../utils/fixture.js' -import fetch from 'node-fetch' import { pause } from '../../utils/pause.js' +import { withSiteBuilder } from '../../utils/site-builder.js' // Skipping tests on Windows because of an issue with the Deno CLI throwing IO // errors when running in the CI. @@ -151,6 +153,47 @@ describe.skipIf(isWindows)('edge functions', () => { }) }) + test('should reload on change to transitive dependency', async (t) => { + await withSiteBuilder(t, async (builder) => { + await builder + .withContentFile({ + path: 'parent.js', + content: "export { foo } from './child.js'", + }) + .withContentFile({ + path: 'child.js', + content: "export const foo = 'foo'", + }) + .withContentFile({ + path: 'netlify/edge-functions/func.js', + content: ` + import { foo } from '../../parent.js' + export default async () => new Response(foo) + export const config = { path: '/' } + `, + }) + .build() + + await withDevServer({ cwd: builder.directory }, async (server) => { + const response = await fetch(server.url, {}).then((res) => res.text()) + t.expect(response).toEqual('foo') + + // update file + await builder + .withContentFile({ + path: 'child.js', + content: "export const foo = 'bar'", + }) + .build() + + await pause(500) + + const response2 = await fetch(server.url, {}).then((res) => res.text()) + t.expect(response2).toEqual('bar') + }) + }) + }) + setupFixtureTests( 'dev-server-with-edge-functions-and-npm-modules', { devServer: true, mockApi: { routes }, setup }, From c665a6ef26f4b6974749fc869d65622c042233e1 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 17:27:37 +0100 Subject: [PATCH 03/10] refactor: if linter tells me to change this, I do it! --- src/lib/edge-functions/registry.ts | 42 ++++++++++++++---------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 700f4742c37..23441e8ab90 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -50,30 +50,28 @@ function traverseLocalDependencies( { dependencies = [] }: ModuleJson, modulesByPath: Map, ): string[] { - return dependencies - .map((dependency) => { - // We're interested in tracking local dependencies, so we only look at - // specifiers with the `file:` protocol. - if ( - dependency.code === undefined || - typeof dependency.code.specifier !== 'string' || - !dependency.code.specifier.startsWith('file://') - ) { - return [] - } - const { specifier: dependencyURL } = dependency.code - const dependencyPath = fileURLToPath(dependencyURL) - const dependencyModule = modulesByPath.get(dependencyPath) + return dependencies.flatMap((dependency) => { + // We're interested in tracking local dependencies, so we only look at + // specifiers with the `file:` protocol. + if ( + dependency.code === undefined || + typeof dependency.code.specifier !== 'string' || + !dependency.code.specifier.startsWith('file://') + ) { + return [] + } + const { specifier: dependencyURL } = dependency.code + const dependencyPath = fileURLToPath(dependencyURL) + const dependencyModule = modulesByPath.get(dependencyPath) - // No module indexed for this dependency - if (dependencyModule === undefined) { - return [dependencyPath] - } + // No module indexed for this dependency + if (dependencyModule === undefined) { + return [dependencyPath] + } - // Keep traversing the child dependencies and return the current dependency path - return [...traverseLocalDependencies(dependencyModule, modulesByPath), dependencyPath] - }) - .reduce((acc, deps) => [...acc, ...deps], []) + // Keep traversing the child dependencies and return the current dependency path + return [...traverseLocalDependencies(dependencyModule, modulesByPath), dependencyPath] + }) } export class EdgeFunctionsRegistry { From f8be5c0528f6cff0216b25fec99da6611bd1be84 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 17:27:58 +0100 Subject: [PATCH 04/10] fix: watch project directory for changes, even if there's no netlify toml --- src/lib/edge-functions/registry.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 23441e8ab90..886bf422c11 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -551,6 +551,12 @@ export class EdgeFunctionsRegistry { } private async setupWatchers(projectDir: string) { + // While functions are guaranteed to be inside one of the configured + // directories, they might be importing files that are located in + // parent directories. So we watch the entire project directory for + // changes. + await this.setupWatcherForDirectory(projectDir) + if (!this.configPath) { return } @@ -566,12 +572,6 @@ export class EdgeFunctionsRegistry { await this.checkForAddedOrDeletedFunctions() }, }) - - // While functions are guaranteed to be inside one of the configured - // directories, they might be importing files that are located in - // parent directories. So we watch the entire project directory for - // changes. - await this.setupWatcherForDirectory(projectDir) } private async setupWatcherForDirectory(directory: string) { From 41360972de6d239bbc974ff7032a5e5282f2cfaf Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:10:34 +0100 Subject: [PATCH 05/10] refactor: remove some duplication --- src/lib/edge-functions/registry.ts | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 886bf422c11..3bdc21aa5ea 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -454,15 +454,12 @@ export class EdgeFunctionsRegistry { // Mapping file URLs to names of functions that use them as dependencies. const dependencyPaths = new Map() - // Map modules by path, acting as an helper index + // Mapping file URLs to modules. Used by the traversal function. const modulesByPath = new Map() - // Map function modules, acting as an helper index - const functionModules = new Map() - - const { modules } = graph - - modules.forEach((module) => { + // a set of edge function modules that we'll use to start traversing the dependency tree from + const functionModules = new Set<{ functionName: string, module: ModuleJson }>() + graph.modules.forEach((module) => { // We're interested in tracking local dependencies, so we only look at // specifiers with the `file:` protocol. const { specifier } = module @@ -470,24 +467,18 @@ export class EdgeFunctionsRegistry { return } - // Populate the module index const path = fileURLToPath(specifier) modulesByPath.set(path, module) - // Populate the function modules index - const functionMatch = functionPaths.get(path) - if (functionMatch) { - functionModules.set(path, module) + const functionName = functionPaths.get(path) + if (functionName) { + functionModules.add({ functionName, module }) } }) // We start from our functions and we traverse through their dependency tree - functionModules.forEach((functionModule) => { - const { specifier } = functionModule - const traversedPaths = traverseLocalDependencies(functionModule, modulesByPath) - const path = fileURLToPath(specifier) - const functionName = functionPaths.get(path) - if (!functionName) return + functionModules.forEach(({ functionName, module }) => { + const traversedPaths = traverseLocalDependencies(module, modulesByPath) // With the paths for the local dependencies we build the dependency path map traversedPaths.forEach((dependencyPath) => { const functions = dependencyPaths.get(dependencyPath) || [] From 4eb1e19ac71a1ea34d08ad564b1de468ac68f581 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:12:35 +0100 Subject: [PATCH 06/10] refactor: extract multimap --- src/lib/edge-functions/registry.ts | 12 +++++------- src/utils/multimap.ts | 11 +++++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) create mode 100644 src/utils/multimap.ts diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 3bdc21aa5ea..19b30115c0e 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -13,6 +13,7 @@ import { watchDebounced, isNodeError, } from '../../utils/command-helpers.js' +import { MultiMap } from '../../utils/multimap.js' // TODO: Replace with a proper type for the entire config object. interface Config { @@ -80,7 +81,7 @@ export class EdgeFunctionsRegistry { private configPath: string private declarationsFromDeployConfig: Declaration[] private declarationsFromTOML: Declaration[] - private dependencyPaths = new Map() + private dependencyPaths = new MultiMap() private directories: string[] private directoryWatchers = new Map() private env: Record @@ -122,7 +123,6 @@ export class EdgeFunctionsRegistry { this.buildError = null this.directoryWatchers = new Map() - this.dependencyPaths = new Map() this.functionPaths = new Map() this.userFunctions = [] this.internalFunctions = [] @@ -452,13 +452,13 @@ export class EdgeFunctionsRegistry { const functionPaths = new Map(Array.from(this.functions, (func) => [func.path, func.name])) // Mapping file URLs to names of functions that use them as dependencies. - const dependencyPaths = new Map() + const dependencyPaths = new MultiMap() // Mapping file URLs to modules. Used by the traversal function. const modulesByPath = new Map() // a set of edge function modules that we'll use to start traversing the dependency tree from - const functionModules = new Set<{ functionName: string, module: ModuleJson }>() + const functionModules = new Set<{ functionName: string; module: ModuleJson }>() graph.modules.forEach((module) => { // We're interested in tracking local dependencies, so we only look at // specifiers with the `file:` protocol. @@ -479,10 +479,8 @@ export class EdgeFunctionsRegistry { // We start from our functions and we traverse through their dependency tree functionModules.forEach(({ functionName, module }) => { const traversedPaths = traverseLocalDependencies(module, modulesByPath) - // With the paths for the local dependencies we build the dependency path map traversedPaths.forEach((dependencyPath) => { - const functions = dependencyPaths.get(dependencyPath) || [] - dependencyPaths.set(dependencyPath, [...functions, functionName]) + dependencyPaths.add(dependencyPath, functionName) }) }) diff --git a/src/utils/multimap.ts b/src/utils/multimap.ts new file mode 100644 index 00000000000..9ca5e9430a7 --- /dev/null +++ b/src/utils/multimap.ts @@ -0,0 +1,11 @@ +export class MultiMap { + private map = new Map() + + add(key: K, value: V) { + this.map.set(key, [...(this.map.get(key) ?? []), value]) + } + + get(key: K): readonly V[] { + return this.map.get(key) ?? [] + } +} From 4b8c4bec7de209dbbb672093d3f511e49541cf9e Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:15:12 +0100 Subject: [PATCH 07/10] refactor: dont keep variables with same name as class variables --- src/lib/edge-functions/registry.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 19b30115c0e..dd00971160b 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -81,7 +81,10 @@ export class EdgeFunctionsRegistry { private configPath: string private declarationsFromDeployConfig: Declaration[] private declarationsFromTOML: Declaration[] + + // Mapping file URLs to names of functions that use them as dependencies. private dependencyPaths = new MultiMap() + private directories: string[] private directoryWatchers = new Map() private env: Record @@ -449,10 +452,9 @@ export class EdgeFunctionsRegistry { // names. This allows us to match modules against functions in O(1) time as // opposed to O(n). // eslint-disable-next-line unicorn/prefer-spread - const functionPaths = new Map(Array.from(this.functions, (func) => [func.path, func.name])) + this.functionPaths = new Map(Array.from(this.functions, (func) => [func.path, func.name])) - // Mapping file URLs to names of functions that use them as dependencies. - const dependencyPaths = new MultiMap() + this.dependencyPaths = new MultiMap() // Mapping file URLs to modules. Used by the traversal function. const modulesByPath = new Map() @@ -470,7 +472,7 @@ export class EdgeFunctionsRegistry { const path = fileURLToPath(specifier) modulesByPath.set(path, module) - const functionName = functionPaths.get(path) + const functionName = this.functionPaths.get(path) if (functionName) { functionModules.add({ functionName, module }) } @@ -480,12 +482,9 @@ export class EdgeFunctionsRegistry { functionModules.forEach(({ functionName, module }) => { const traversedPaths = traverseLocalDependencies(module, modulesByPath) traversedPaths.forEach((dependencyPath) => { - dependencyPaths.add(dependencyPath, functionName) + this.dependencyPaths.add(dependencyPath, functionName) }) }) - - this.dependencyPaths = dependencyPaths - this.functionPaths = functionPaths } /** From bb5f85d2578fa2c02dc88c4ff37e8c11e5a0bb14 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:17:41 +0100 Subject: [PATCH 08/10] refactor: create functionPaths closer to where the underlying data is updated --- src/lib/edge-functions/registry.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index dd00971160b..3d38a01ea96 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -88,16 +88,22 @@ export class EdgeFunctionsRegistry { private directories: string[] private directoryWatchers = new Map() private env: Record + + private userFunctions: EdgeFunction[] = [] + private internalFunctions: EdgeFunction[] = [] + + // a Map from `this.functions` that maps function paths to function + // names. This allows us to match modules against functions in O(1) time as + // opposed to O(n). private functionPaths = new Map() + private getUpdatedConfig: () => Promise private initialScan: Promise private internalDirectories: string[] - private internalFunctions: EdgeFunction[] = [] private manifest: Manifest | null = null private routes: Route[] = [] private runIsolate: RunIsolate private servePath: string - private userFunctions: EdgeFunction[] = [] constructor({ bundler, @@ -448,12 +454,6 @@ export class EdgeFunctionsRegistry { return } - // Creating a Map from `this.functions` that maps function paths to function - // names. This allows us to match modules against functions in O(1) time as - // opposed to O(n). - // eslint-disable-next-line unicorn/prefer-spread - this.functionPaths = new Map(Array.from(this.functions, (func) => [func.path, func.name])) - this.dependencyPaths = new MultiMap() // Mapping file URLs to modules. Used by the traversal function. @@ -535,6 +535,9 @@ export class EdgeFunctionsRegistry { this.internalFunctions = internalFunctions this.userFunctions = userFunctions + // eslint-disable-next-line unicorn/prefer-spread + this.functionPaths = new Map(Array.from(this.functions, (func) => [func.path, func.name])) + return { all: functions, new: newFunctions, deleted: deletedFunctions } } From 954f2ba40b261768b82a58eafdaaa7f7c1737cc8 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:18:30 +0100 Subject: [PATCH 09/10] refactor: remove useless initialisers --- src/lib/edge-functions/registry.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index 3d38a01ea96..4119873e96c 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -130,12 +130,6 @@ export class EdgeFunctionsRegistry { this.declarationsFromTOML = EdgeFunctionsRegistry.getDeclarationsFromTOML(config) this.env = EdgeFunctionsRegistry.getEnvironmentVariables(env) - this.buildError = null - this.directoryWatchers = new Map() - this.functionPaths = new Map() - this.userFunctions = [] - this.internalFunctions = [] - this.initialScan = this.doInitialScan() this.setupWatchers(projectDir) From b405e77f18c6e9af5541285caab1371aa443c7c6 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 8 Jan 2024 18:34:26 +0100 Subject: [PATCH 10/10] chore: prettier --- src/lib/edge-functions/registry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/edge-functions/registry.ts b/src/lib/edge-functions/registry.ts index d5d4b69967e..bc236ad1acb 100644 --- a/src/lib/edge-functions/registry.ts +++ b/src/lib/edge-functions/registry.ts @@ -102,7 +102,7 @@ export class EdgeFunctionsRegistry { // names. This allows us to match modules against functions in O(1) time as // opposed to O(n). private functionPaths = new Map() - + private getUpdatedConfig: () => Promise private initialScan: Promise private manifest: Manifest | null = null