deps: update fastify to v4.26.2 #6626
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeluard
previously approved these changes
Apr 3, 2024
nflaig
commented
Apr 3, 2024
| @@ -84,34 +84,34 @@ export class RestApiServer { | |||
| // Log all incoming request to debug (before parsing). TODO: Should we hook latter in the lifecycle? https://www.fastify.io/docs/latest/Lifecycle/ | |||
| // Note: Must be an async method so fastify can continue the release lifecycle. Otherwise we must call done() or the request stalls | |||
| server.addHook("onRequest", async (req, _res) => { | |||
| const {operationId} = req.routeConfig as RouteConfig; | |||
| this.logger.debug(`Req ${req.id as string} ${req.ip} ${operationId}`); | |||
| const operationId = req.routeSchema.operationId as string; | |||
There was a problem hiding this comment.
It makes more sense to get this from schema as operationId is already part of standard type definition. We can drop including it in custom routeConfig altogether but will do that on the refactor branch
| metrics?.responseTime.observe({operationId}, res.getResponseTime() / 1000); | ||
| const operationId = req.routeSchema.operationId as string; | ||
| this.logger.debug(`Res ${req.id} ${operationId} - ${res.raw.statusCode}`); | ||
| metrics?.responseTime.observe({operationId}, res.elapsedTime / 1000); |
There was a problem hiding this comment.
As of latest release getResponseTime() has been deprecated (fastify/fastify#5263) and will be removed in the next major release (v5)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6626 +/- ##
=========================================
Coverage 61.70% 61.70%
=========================================
Files 556 556
Lines 58799 58799
Branches 1886 1886
=========================================
Hits 36283 36283
Misses 22475 22475
Partials 41 41 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
jeluard
previously approved these changes
Apr 4, 2024
Good catch! I think this PR is a good place to fix this, also updated for light-client package which uses fastify in tests as well
Will do some testing, haven't looked at actual performance difference yet |
jeluard
approved these changes
Apr 4, 2024
|
🎉 This PR is included in v1.18.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The latest version includes proper support for returning
Uint8Arrayfrom handler and will efficiently convert it to aBufferinternally without copying the underlyingArrayBuffer.The current approach we use is copying it instead which is not ideal
lodestar/packages/api/src/beacon/server/beacon.ts
Line 26 in 5577996
But I think we can keep this as is for now. I mainly want to test against latest fastify version on the api client refactor branch without increasing the diff there even more by updating dependencies.
Description
Update fastify to v4.26.2 which includes fix for returning
Uint8Arrayfrom handlers (fastify/fastify#5124)