feat(edge-bundler): exclude unrouted functions from the bundle - #7132
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e18e dependency analysisNo dependency warnings found. |
@netlify/build
@netlify/build-info
@netlify/cache-utils
@netlify/config
@netlify/edge-bundler
@netlify/functions-utils
@netlify/git-utils
@netlify/headers-parser
@netlify/api
@netlify/nock-udp
@netlify/opentelemetry-sdk-setup
@netlify/opentelemetry-utils
@netlify/redirect-parser
@netlify/run-utils
@netlify/zip-it-and-ship-it
commit: |
72a41e3 to
361909f
Compare
361909f to
25eac21
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/build/tests/edge_functions/snapshots/tests.js.md`:
- Around line 1315-1319: Update the stderr snapshot string in the affected test
fixture to use escaped newline characters (\n) rather than literal /n sequences,
including the separator before the source excerpt and its subsequent lines.
Preserve the existing error text and formatting so it matches the actual
multiline stderr output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf093cd8-0406-4d0e-981c-d4d750539418
⛔ Files ignored due to path filters (1)
packages/build/tests/edge_functions/snapshots/tests.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/build/tests/edge_functions/snapshots/tests.js.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
|
|
||
| await vendor?.cleanup() | ||
|
|
||
| return { functions, manifest: undefined } |
There was a problem hiding this comment.
I think this is correct thing to do, just noting that it changes signature of this function and it is exported from @netlify/edge-bundler -
before (i.e. https://npmx.dev/package-code/@netlify/edge-bundler/v/15.1.1/dist%2Fnode%2Fbundler.d.ts#L23-L26):
export declare const bundle: (sourceDirectories: string[], distDirectory: string, tomlDeclarations?: Declaration[], { basePath: inputBasePath, cacheDirectory, configPath, debug, distImportMapPath, featureFlags: inputFeatureFlags, importMapPaths, internalSrcFolder, onAfterDownload, onBeforeDownload, rootPath, userLogger, systemLogger, vendorDirectory, }?: BundleOptions) => Promise<{
functions: EdgeFunction[];
manifest: import("./manifest.js").Manifest;
}>;after:
export declare const bundle: (sourceDirectories: string[], distDirectory: string, tomlDeclarations?: Declaration[], { basePath: inputBasePath, cacheDirectory, configPath, debug, distImportMapPath, featureFlags: inputFeatureFlags, importMapPaths, internalSrcFolder, onAfterDownload, onBeforeDownload, rootPath, userLogger, systemLogger, vendorDirectory, }?: BundleOptions) => Promise<{
functions: EdgeFunction[];
manifest: undefined;
} | {
functions: EdgeFunction[];
manifest: import("./manifest.js").Manifest;
}>;I think this warrants major version bump for @netlify/edge-bundler just to be safe. As far as I'm aware @netlify/build is only consumer of it, but I'm not 100% sure on this.
The annoying part here is that I'm not sure if release-please have a way to only bump major for just @netlify/edge-bundler without bumping it also for @netlify/build which does NOT need major bump. I'll try to figure this out without doing some weird PR splits so release-please is satisfied.
There was a problem hiding this comment.
@pieh if we didn't do a merge commit or squash merge, then it work because each commit can have a different feat/fix etc
| logFunctionsToBundle({ | ||
| logs, | ||
| userFunctions: userFunctions.map(({ name }) => name), | ||
| userFunctionsSrc: srcDirectory, | ||
| userFunctionsSrcExists, | ||
| internalFunctions: internalFunctions.map(({ name }) => name), | ||
| internalFunctionsSrc: internalSrcDirectory, | ||
| frameworkFunctions: frameworkFunctions.map(({ name }) => name), | ||
| type: 'Edge Functions', | ||
| generatedFunctions: {}, | ||
| }) |
There was a problem hiding this comment.
Not blocking (IMO), just something that noticed when testing this change that seems worth enough to mention
In case I have no routable EF I do get those logs
Edge Functions bundling
────────────────────────────────────────────────────────────────
Packaging Edge Functions from netlify/edge-functions directory:
- wat
(Edge Functions bundling completed in 1.6s)
Without getting feedback that this particular EF was not actually "packaged".
I don't think this is exactly a regression in practice even if "technically" it might be considered one, because today if the EF is not routable - while EF is packaged - but's never invokable, so in practice wether EF was packaged or skipped doesn't seem to matter in practice - in both cases it just will never run.
We maybe could use this opportunity to add a note/warning mentioning that EF does not have any routes and it will never be invoked and have call to action for user to either add routing OR if the module was listed here but is not meant to be actual EF - do something to not be considered EF candidate (this is part is main reason to not even try to do that in this PR because when I was looking for our documentation - the logic used to determine what files should be considered edge functions https://github.com/netlify/build/blob/main/packages/edge-bundler/node/finder.ts doesn't seem to be properly documented so we could not refer to documentation even if we wanted to without adding a section about it first)
e14f077 to
5322d53
Compare
A function with no route can never be invoked, so bundling it only eagerly loads code that can never run, which can surface import-time failures for functions that are not even in use. Behind the `edge_bundler_exclude_unrouted_functions` feature flag, unrouted functions are left out of the bundle, and when nothing is left to bundle we produce neither a bundle nor a manifest. BREAKING CHANGE: the `manifest` property returned by `bundle` is now `Manifest | undefined`. It is `undefined` when no function is bundled.
5322d53 to
d248d31
Compare
2 participants
Summary
Functions with no route are never invoked, but they were still bundled and eagerly loaded, which could surface import-time failures Behind the
edge_bundler_exclude_unrouted_functionsflag, compute routes before bundling and leave unrouted functions out of the tarball and ESZIPhttps://linear.app/netlify/issue/FRB-2238/exclude-unconfigured-edge-functions-from-nimble-bundles