Skip to content

fix: handling for empty cases in edge bundling #6657

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix: handling for empty cases in edge bundling #6657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f79ca4
test: assert that no bundles are produced when edge function director…
mrstork Sep 9, 2025
7782f6d
fix: handle empty cases in edge bundling
mrstork Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/build/src/plugins_core/edge_functions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ const coreStep = async function ({
bootstrapURL: edgeFunctionsBootstrapURL,
vendorDirectory,
})

// There were no functions to bundle, can exit this step early
if (!manifest) return {}

const metrics = getMetrics(manifest)
Copy link
Member

Choose a reason for hiding this comment

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

by returning early, we're no longer recording these metrics (on line 152–153) with zeros. this might trigger some alerts or require some adjustments / context sharing. maybe we could avoid this change to keep things simple? and to allow tracking number of builds with no EFs

Copy link
Contributor

Choose a reason for hiding this comment

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

We also completely skip the entire step already of .netlify/edge-functions, netlify/edge-functions (or custom user one) and frameworks API one doesn't exist and don't report those metrics then.

This is overall similar scenario. We pondered about expanding current condition:

build/packages/build/src/plugins_core/edge_functions/index.ts

Lines 156 to 180 in 7782f6d

// We run this core step if at least one of the functions directories (the
// one configured by the user or the internal one) exists. We use a dynamic
// `condition` because the directories might be created by the build command
// or plugins.
const hasEdgeFunctionsDirectories = async function ({
buildDir,
constants: { INTERNAL_EDGE_FUNCTIONS_SRC, EDGE_FUNCTIONS_SRC },
packagePath,
}): Promise<boolean> {
const hasFunctionsSrc = EDGE_FUNCTIONS_SRC !== undefined && EDGE_FUNCTIONS_SRC !== ''
if (hasFunctionsSrc) {
return true
}
const internalFunctionsSrc = resolve(buildDir, INTERNAL_EDGE_FUNCTIONS_SRC)
if (await pathExists(internalFunctionsSrc)) {
return true
}
const frameworkFunctionsSrc = resolve(buildDir, packagePath || '', FRAMEWORKS_API_EDGE_FUNCTIONS_PATH)
return await pathExists(frameworkFunctionsSrc)
}

and try to find functions there already and skip entire step then, but it did feel a bit wasteful to repeat steps of trying to find edge functions multiple times and opted to do "early bail" inside step itself using edge functions search that we already do


systemLog('Edge Functions manifest:', manifest)
Expand Down
Empty file added .../tests/edge_functions/fixtures/functions_empty_directory/.netlify/edge-functions/.gitkeep
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions ...ts/edge_functions/fixtures/functions_empty_manifest/.netlify/edge-functions/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"functions": [],
"version": 1
}
28 changes: 28 additions & 0 deletions packages/build/tests/edge_functions/tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,3 +289,31 @@ test.serial(
])
},
)

test('skip bundling when edge function directories exist, contain no functions', async (t) => {
await new Fixture('./fixtures/functions_empty_directory').runWithBuild()

const manifestPath = join(
FIXTURES_DIR,
'functions_empty_directory',
'.netlify',
'edge-functions-dist',
'manifest.json',
)

t.false(await pathExists(manifestPath))
})

test('skip bundling when edge function directories exist, contain no functions, contain empty manifest', async (t) => {
await new Fixture('./fixtures/functions_empty_manifest').runWithBuild()

const manifestPath = join(
FIXTURES_DIR,
'functions_empty_manifest',
'.netlify',
'edge-functions-dist',
'manifest.json',
)

t.false(await pathExists(manifestPath))
})
20 changes: 10 additions & 10 deletions packages/edge-bundler/node/bundler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ test('Produces an ESZIP bundle', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(3)
expect(result.functions?.length).toBe(3)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -74,7 +74,7 @@ test('Uses the vendored eszip module instead of fetching it from deno.land', asy
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -182,7 +182,7 @@ test('Uses the cache directory as the `DENO_DIR` value', async () => {
const result = await bundle([sourceDirectory], distPath, declarations, options)
const outFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(outFiles.length).toBe(2)

const denoDir = await readdir(join(cacheDir.path, 'deno_dir'))
Expand All @@ -207,7 +207,7 @@ test('Supports import maps with relative paths', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -293,7 +293,7 @@ test('Processes a function that imports a custom layer', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -325,7 +325,7 @@ test('Loads declarations and import maps from the deploy configuration and in-so
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(3)
expect(result.functions?.length).toBe(3)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -391,7 +391,7 @@ test("Ignores entries in `importMapPaths` that don't point to an existing import
)
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(2)
expect(result.functions?.length).toBe(2)
expect(generatedFiles.length).toBe(2)
expect(systemLogger).toHaveBeenCalledWith(`Did not find an import map file at '${nonExistingImportMapPath}'.`)

Expand All @@ -408,7 +408,7 @@ test('Handles imports with the `node:` prefix', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -444,7 +444,7 @@ test('Handles Node builtin imports without the `node:` prefix', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(1)
expect(result.functions?.length).toBe(1)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down Expand Up @@ -706,7 +706,7 @@ test('Loads edge functions from the Frameworks API', async () => {
})
const generatedFiles = await readdir(distPath)

expect(result.functions.length).toBe(3)
expect(result.functions?.length).toBe(3)
expect(generatedFiles.length).toBe(2)

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down
4 changes: 4 additions & 0 deletions packages/edge-bundler/node/bundler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ export const bundle = async (
const userFunctions = userSourceDirectories.length === 0 ? [] : await findFunctions(userSourceDirectories)
const internalFunctions = internalSrcFolder ? await findFunctions(internalSrcFolders) : []
const functions = [...internalFunctions, ...userFunctions]

// Early exit when there are no functions to bundle
if (functions.length === 0) return {}
Copy link
Member

Choose a reason for hiding this comment

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

this results in a breaking change to the signature of the bundle() function exported from this package. if that's intentional, we should mark this PR as breaking (though note that since it changes two packages it'll cut a major for both).

(btw personal opinion but this is why I like https://typescript-eslint.io/rules/explicit-function-return-type/. it forces changes to return types to be explicit and visible.)

Copy link
Contributor Author

@mrstork mrstork Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are you thinking this shouldn't be touched and we should only make the change on the adapter side of things? Just trying my best to understand what the way forward is that is being suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it make sense try not to touch edge-bundler and in particular change signature if possible here. I did take a look at how we handle this for serverless and we have similar early bail there, but it is in step itself instead of bundler/zisi and applied similar change in #6659 which is alternative to this PR - no breaking changes there and no additional file lookups


const vendor = await safelyVendorNPMSpecifiers({
basePath,
featureFlags,
Expand Down
2 changes: 1 addition & 1 deletion packages/edge-bundler/node/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ test('Loads function paths from the in-source `config` function', async () => {
})
const generatedFiles = await fs.readdir(distPath)

expect(result.functions.length).toBe(11)
expect(result.functions?.length).toBe(11)
expect(generatedFiles.length).toBe(2)

const manifestFile = await fs.readFile(resolve(distPath, 'manifest.json'), 'utf8')
Expand Down
Loading