-
Notifications
You must be signed in to change notification settings - Fork 357
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
fix!: execute user and integrations edge functions in the correct order #5624
Conversation
📊 Benchmark resultsComparing with 9066166 Package size: 302 MB⬆️ 0.00% increase vs. 9066166
|
const [internalFunctions, userFunctions] = await Promise.all([ | ||
this.#scanForFunctions(this.#internalDirectories), | ||
this.#scanForFunctions(this.#directories), | ||
]) |
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 read files separately for internal vs. user
|
||
this.#userFunctionConfigs = this.#userFunctions.reduce( | ||
// eslint-disable-next-line no-plusplus | ||
(acc, func) => ({ ...acc, [func.name]: functionsConfig[index++] }), |
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 create separate functionConfigs for user vs internal
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.
I'm curious why you decided to put in the work to transform the fields to private fields? Is it necessary in order for this fix to work?
No it wasn't strictly necessary, but as I moved things around inside the registry I was never sure if this method/property is public or private. I know it makes the PR a little hard to review. |
Yeah I think in general it makes the code harder to read, which makes me wonder if it is worth the trade-off. |
Summary
This turned out to be a bigger change than anticipated. The main change is splitting up the directories in user and internal, so that we can call
bundler.find
for each of them. We still have to run them together, but the functionConfig returned frombundler.serve
is again split up, so that we can generate the correct manifest with the correct order.Soon to be documented:
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)