fix: handle case of zero-length cacheable route handler responses #2819
Conversation
This file contains hidden or 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
📊 Package size report 0.02%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
…ory cache failures not fatal
pieh
commented
Apr 7, 2025
| } | ||
| let knownKindFailed = false | ||
| if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { | ||
| return BASE_BLOB_SIZE + valueToStore.value.body.length |
There was a problem hiding this comment.
This in particular is the fix for added test case.
But using it as example, I did add more adjustments:
- ensure we add "base" blob size to all known cases
- ensure
estimateBlobSizereturns "PositiveNumber" - make using in-memory store non-fatal - this is optimization, but failures related to it should NOT cause crashes. I did add recording of failures there so we can adjust code if problems happen still
orinokai
previously approved these changes
Apr 7, 2025
| } | ||
|
|
||
| const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): PositiveNumber => { | ||
| let knownTypeFailed = false |
There was a problem hiding this comment.
Technically, you could omit this var now that you're using estimateBlobKnownTypeSizeError. It's only used in the warning message below, which could be replaced by checking if estimateBlobKnownTypeSizeError is not undefined, but it's not a blocker, just something to consider next time you're touching the code here.
There was a problem hiding this comment.
I'll adjust now - it's unlikely this will be done as follow up
There was a problem hiding this comment.
f1ab628 dropped that extra unneded variable. Also added calculated size to warning just for it to have a bit more information
orinokai
approved these changes
Apr 7, 2025
pieh
deleted the
michalpiechowiak/frb-1721-sizecalculation-return-invalid-expect-positive-integer
branch
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.
Description
Size calculcation for in-memory store in some cases was returning 0 (in particular known case is cacheable route handler with 0-length body).
This PR does few things:
Tests
Added known case that is causing errors with currently released version
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRB-1721/sizecalculation-return-invalid-expect-positive-integer