From 71702b35f8b0340d4abfbaf89c353c9a76a0aba6 Mon Sep 17 00:00:00 2001 From: jake champion Date: Thu, 10 Apr 2025 12:17:05 +0100 Subject: [PATCH 1/2] fix(edge-bundler): only parse a file once for it's npm specifiers --- .../edge-bundler/node/npm_dependencies.ts | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/edge-bundler/node/npm_dependencies.ts b/packages/edge-bundler/node/npm_dependencies.ts index e57af5fd14..fa810482ca 100644 --- a/packages/edge-bundler/node/npm_dependencies.ts +++ b/packages/edge-bundler/node/npm_dependencies.ts @@ -145,7 +145,10 @@ async function parseImportsForFile(file: string, rootPath: string) { * Parses a set of functions and returns a list of specifiers that correspond * to npm modules. */ -const getNPMSpecifiers = async ({ basePath, functions, importMap, environment, rootPath }: GetNPMSpecifiersOptions) => { +const getNPMSpecifiers = async ( + { basePath, functions, importMap, environment, rootPath }: GetNPMSpecifiersOptions, + alreadySeenPaths = new Set(), +) => { const baseURL = pathToFileURL(basePath) const npmSpecifiers: { specifier: string; types?: string }[] = [] for (const func of functions) { @@ -158,14 +161,24 @@ const getNPMSpecifiers = async ({ basePath, functions, importMap, environment, r switch (i.moduleSpecifier.type) { case 'absolute': { npmSpecifiers.push( - ...(await getNPMSpecifiers({ basePath, functions: [specifier], importMap, environment, rootPath })), + ...(await getNPMSpecifiers( + { basePath, functions: [specifier], importMap, environment, rootPath }, + alreadySeenPaths, + )), ) break } case 'relative': { const filePath = path.join(path.dirname(func), specifier) + if (alreadySeenPaths.has(filePath)) { + break + } + alreadySeenPaths.add(filePath) npmSpecifiers.push( - ...(await getNPMSpecifiers({ basePath, functions: [filePath], importMap, environment, rootPath })), + ...(await getNPMSpecifiers( + { basePath, functions: [filePath], importMap, environment, rootPath }, + alreadySeenPaths, + )), ) break } @@ -181,7 +194,10 @@ const getNPMSpecifiers = async ({ basePath, functions, importMap, environment, r if (resolvedImport.protocol === 'file:') { const newSpecifier = fileURLToPath(resolvedImport).replace(/\\/g, '/') npmSpecifiers.push( - ...(await getNPMSpecifiers({ basePath, functions: [newSpecifier], importMap, environment, rootPath })), + ...(await getNPMSpecifiers( + { basePath, functions: [newSpecifier], importMap, environment, rootPath }, + alreadySeenPaths, + )), ) } } else if (!resolvedImport?.protocol?.startsWith('http')) { From 0db47463fc081f7e89129188b623e585ba24ed5e Mon Sep 17 00:00:00 2001 From: jake champion Date: Thu, 10 Apr 2025 12:38:59 +0100 Subject: [PATCH 2/2] chore: add test and more break cases --- packages/edge-bundler/node/bundler.test.ts | 27 +++++++++++++++++++ .../edge-bundler/node/npm_dependencies.ts | 8 ++++++ .../fixtures/imports_cycle/functions/func1.ts | 7 +++++ .../test/fixtures/imports_cycle/hello.ts | 5 ++++ .../test/fixtures/imports_cycle/package.json | 3 +++ 5 files changed, 50 insertions(+) create mode 100644 packages/edge-bundler/test/fixtures/imports_cycle/functions/func1.ts create mode 100644 packages/edge-bundler/test/fixtures/imports_cycle/hello.ts create mode 100644 packages/edge-bundler/test/fixtures/imports_cycle/package.json diff --git a/packages/edge-bundler/node/bundler.test.ts b/packages/edge-bundler/node/bundler.test.ts index 0883ac6396..c73601c1c1 100644 --- a/packages/edge-bundler/node/bundler.test.ts +++ b/packages/edge-bundler/node/bundler.test.ts @@ -533,6 +533,33 @@ test('Loads npm modules which use package.json.exports', async () => { await rm(vendorDirectory.path, { force: true, recursive: true }) }) +test('Loads modules which contain cycles', async () => { + const { basePath, cleanup, distPath } = await useFixture('imports_cycle') + const sourceDirectory = join(basePath, 'functions') + const declarations: Declaration[] = [ + { + function: 'func1', + path: '/func1', + }, + ] + const vendorDirectory = await tmp.dir() + + await bundle([sourceDirectory], distPath, declarations, { + basePath, + vendorDirectory: vendorDirectory.path, + }) + + const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8') + const manifest = JSON.parse(manifestFile) + const bundlePath = join(distPath, manifest.bundles[0].asset) + const { func1 } = await runESZIP(bundlePath, vendorDirectory.path) + + expect(func1).toBe('magix') + + await cleanup() + await rm(vendorDirectory.path, { force: true, recursive: true }) +}) + test('Loads npm modules in a monorepo setup', async () => { const systemLogger = vi.fn() const { basePath: rootPath, cleanup, distPath } = await useFixture('monorepo_npm_module') diff --git a/packages/edge-bundler/node/npm_dependencies.ts b/packages/edge-bundler/node/npm_dependencies.ts index fa810482ca..6a3e2a92e5 100644 --- a/packages/edge-bundler/node/npm_dependencies.ts +++ b/packages/edge-bundler/node/npm_dependencies.ts @@ -160,6 +160,10 @@ const getNPMSpecifiers = async ( const specifier = i.moduleSpecifier.isConstant ? i.moduleSpecifier.value! : i.moduleSpecifier.code switch (i.moduleSpecifier.type) { case 'absolute': { + if (alreadySeenPaths.has(specifier)) { + break + } + alreadySeenPaths.add(specifier) npmSpecifiers.push( ...(await getNPMSpecifiers( { basePath, functions: [specifier], importMap, environment, rootPath }, @@ -193,6 +197,10 @@ const getNPMSpecifiers = async ( if (matched) { if (resolvedImport.protocol === 'file:') { const newSpecifier = fileURLToPath(resolvedImport).replace(/\\/g, '/') + if (alreadySeenPaths.has(newSpecifier)) { + break + } + alreadySeenPaths.add(newSpecifier) npmSpecifiers.push( ...(await getNPMSpecifiers( { basePath, functions: [newSpecifier], importMap, environment, rootPath }, diff --git a/packages/edge-bundler/test/fixtures/imports_cycle/functions/func1.ts b/packages/edge-bundler/test/fixtures/imports_cycle/functions/func1.ts new file mode 100644 index 0000000000..a35584502c --- /dev/null +++ b/packages/edge-bundler/test/fixtures/imports_cycle/functions/func1.ts @@ -0,0 +1,7 @@ +import hello from '../hello.ts' + +export const name = "magix" + +export default async () => { + return new Response(hello()) +} diff --git a/packages/edge-bundler/test/fixtures/imports_cycle/hello.ts b/packages/edge-bundler/test/fixtures/imports_cycle/hello.ts new file mode 100644 index 0000000000..18f0193345 --- /dev/null +++ b/packages/edge-bundler/test/fixtures/imports_cycle/hello.ts @@ -0,0 +1,5 @@ +import { name } from "./functions/func1.ts"; + +export default function hello() { + return name +} \ No newline at end of file diff --git a/packages/edge-bundler/test/fixtures/imports_cycle/package.json b/packages/edge-bundler/test/fixtures/imports_cycle/package.json new file mode 100644 index 0000000000..3dbc1ca591 --- /dev/null +++ b/packages/edge-bundler/test/fixtures/imports_cycle/package.json @@ -0,0 +1,3 @@ +{ + "type": "module" +}