-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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
Conversation
…ies/manifests do not exist
This pull request adds or modifies JavaScript ( |
// There were no functions to bundle, can exit this step early | ||
if (!manifest) return {} | ||
|
||
const metrics = getMetrics(manifest) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
const functions = [...internalFunctions, ...userFunctions] | ||
|
||
// Early exit when there are no functions to bundle | ||
if (functions.length === 0) return {} |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Summary
These changes handle two empty cases
edge-function
directory exists and is emptyedge-function
directory exists and contains an empty manifestFixes FRB-1985
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)