Correctly deserialize `undefined` unstable_cache data #59126

mattddean · 2023-11-30T15:34:11Z

The value undefined can be saved to the incremental cache as undefined (JSON.stringify(undefined)) with no errors, but when retrieving it, we attempt to parse it as JSON using JSON.parse(undefined). This throws an error. We should instead deserialize undefined as undefined when retrieving.

relevant discussion: #59087

prasad-pul

Looks good!

smaeda-ks · 2023-12-10T00:19:05Z

packages/next/src/server/web/spec-extension/unstable-cache.ts

        if (cacheEntry) {
          const resData = cacheEntry.value.data
-          cachedValue = JSON.parse(resData.body)
+          // JSON.stringify(undefined) returns `undefined`, but JSON.parse(undefined) throws an error
+          cachedValue =
+            resData.body !== undefined ? JSON.parse(resData.body) : undefined
        }


Can we also just move this entire condition below if (isStale) { ... } ?

makes sense, updated

unstable_cache was refactored, but the issue of JSON.stringifying the cache's data that could be undefined, then JSON.parseing it on retrieval persists, so I'm going to leave this PR open.

I'm not sure why is this not being merged or reviewed yet by NextJs team 🤔
Isn't this a critical bug, when an api returns undefined the unstable_cache crashes the Application. Isn't that a very common scenario??

Issue #59124

cc: @mattddean , @ijjk , @timneutkens

stabildev · 2024-02-22T12:24:52Z

important!

prasad-pul · 2024-02-22T12:27:39Z

No idea why this isn't looked up by Next js team??

samcx · 2024-02-22T18:50:16Z

Taking a look!

samcx · 2024-03-08T22:05:29Z

Still looking into this everyone 🙇🏼

mattddean · 2024-03-08T22:07:36Z

Still looking into this everyone 🙇🏼

Happy to help if I can! Or to write a failing test if you point me at the right directory to put it in.

samcx · 2024-03-08T22:24:05Z

@mattddean Yeah we should add a test. Let me see where we should

samcx · 2024-03-08T22:58:35Z

@mattddean You can add a directory called unstable_cache (similar to the no_store directory, but without the unstable_noStore) and add a 1-2 test(s) (one to test unstable_cache is working as expected, and another to test undefined) after the unstable_noStore` describe →

next.js/test/e2e/app-dir/app-static/app-static.test.ts

Line 2991 in a767aa8

describe('unstable_noStore', () => {

Let me know if that helps!

mattddean · 2024-03-09T00:32:40Z

@samcx Super helpful, thanks! Let me know if the tests I've added look sufficient to you.

mattddean · 2024-03-09T00:45:42Z

Quick screenshot of the new test failing before applying this change:

ijjk · 2024-03-09T05:18:37Z

Allow CI Workflow Run

approve CI run for commit: 7b5e8f0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2024-03-09T05:18:37Z

Allow CI Workflow Run

approve CI run for commit: 7b5e8f0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2024-03-09T05:18:37Z

Allow CI Workflow Run

approve CI run for commit: 7b5e8f0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2024-03-09T05:25:22Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	mattddean/next.js patch-2	Change
buildDuration	13.6s	13.6s	N/A
buildDurationCached	7.3s	6.1s	N/A
nodeModulesSize	198 MB	198 MB	⚠️ +924 B
nextStartRea..uration (ms)	429ms	433ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	mattddean/next.js patch-2	Change
2453-HASH.js gzip	30.5 kB	30.5 kB	N/A
3304.HASH.js gzip	181 B	181 B	✓
3f784ff6-HASH.js gzip	53.7 kB	53.7 kB	N/A
8299-HASH.js gzip	5.04 kB	5.04 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	242 B	✓
main-HASH.js gzip	32.2 kB	32.2 kB	N/A
webpack-HASH.js gzip	1.68 kB	1.68 kB	N/A
Overall change	45.6 kB	45.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	mattddean/next.js patch-2	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	mattddean/next.js patch-2	Change
_app-HASH.js gzip	196 B	197 B	N/A
_error-HASH.js gzip	184 B	184 B	✓
amp-HASH.js gzip	505 B	505 B	✓
css-HASH.js gzip	324 B	325 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	258 B	258 B	✓
head-HASH.js gzip	352 B	352 B	✓
hooks-HASH.js gzip	370 B	371 B	N/A
image-HASH.js gzip	4.21 kB	4.21 kB	✓
index-HASH.js gzip	259 B	259 B	✓
link-HASH.js gzip	2.67 kB	2.67 kB	N/A
routerDirect..HASH.js gzip	314 B	312 B	N/A
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	6.57 kB	6.57 kB	✓

Client Build Manifests

	vercel/next.js canary	mattddean/next.js patch-2	Change
_buildManifest.js gzip	481 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	mattddean/next.js patch-2	Change
index.html gzip	529 B	529 B	✓
link.html gzip	541 B	542 B	N/A
withRouter.html gzip	524 B	523 B	N/A
Overall change	529 B	529 B	✓

Edge SSR bundle Size

	vercel/next.js canary	mattddean/next.js patch-2	Change
edge-ssr.js gzip	95.1 kB	95.1 kB	N/A
page.js gzip	3.07 kB	3.07 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	mattddean/next.js patch-2	Change
middleware-b..fest.js gzip	624 B	624 B	✓
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	25.5 kB	25.5 kB	N/A
edge-runtime..pack.js gzip	839 B	839 B	✓
Overall change	1.61 kB	1.61 kB	✓

Next Runtimes

	vercel/next.js canary	mattddean/next.js patch-2	Change
app-page-exp...dev.js gzip	171 kB	171 kB	✓
app-page-exp..prod.js gzip	96.5 kB	96.5 kB	✓
app-page-tur..prod.js gzip	98.3 kB	98.3 kB	✓
app-page-tur..prod.js gzip	92.7 kB	92.7 kB	✓
app-page.run...dev.js gzip	149 kB	149 kB	✓
app-page.run..prod.js gzip	91.2 kB	91.2 kB	✓
app-route-ex...dev.js gzip	21.3 kB	21.3 kB	✓
app-route-ex..prod.js gzip	15 kB	15 kB	✓
app-route-tu..prod.js gzip	15 kB	15 kB	✓
app-route-tu..prod.js gzip	14.8 kB	14.8 kB	✓
app-route.ru...dev.js gzip	21 kB	21 kB	✓
app-route.ru..prod.js gzip	14.8 kB	14.8 kB	✓
pages-api-tu..prod.js gzip	9.52 kB	9.52 kB	✓
pages-api.ru...dev.js gzip	9.8 kB	9.8 kB	✓
pages-api.ru..prod.js gzip	9.52 kB	9.52 kB	✓
pages-turbo...prod.js gzip	22.3 kB	22.3 kB	✓
pages.runtim...dev.js gzip	22.9 kB	22.9 kB	✓
pages.runtim..prod.js gzip	22.3 kB	22.3 kB	✓
server.runti..prod.js gzip	50.5 kB	50.5 kB	✓
Overall change	948 kB	948 kB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	mattddean/next.js patch-2	Change
0.pack gzip	1.56 MB	1.55 MB	N/A
index.pack gzip	105 kB	106 kB	⚠️ +464 B
Overall change	105 kB	106 kB	⚠️ +464 B

Diff details

Diff for middleware.js

Diff too large to display

Commit: f510913

ijjk · 2024-03-09T05:25:34Z

Tests Passed

Facundo-Martin · 2024-03-12T20:59:56Z

Haha just ran into this bug today at work. It's nice to see it's been fixed! Although just for the canary version I guess.
My solution was to just return null instead of undefined.

mattddean added 2 commits November 30, 2023 08:20

Correctly deserialize undefined unstable_cache data.

7e3e944

format

c668a9c

mattddean requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners November 30, 2023 15:34

ijjk added the type: next label Nov 30, 2023

mattddean changed the title ~~Correctly deserialize undefined unstable_cache data.~~ Correctly deserialize undefined unstable_cache data Nov 30, 2023

prasad-pul approved these changes Dec 1, 2023

View reviewed changes

smaeda-ks reviewed Dec 10, 2023

View reviewed changes

check isStale before attempting to deserialize cached value

5fb06c8

mattddean requested a review from smaeda-ks December 11, 2023 15:30

mattddean added 2 commits January 15, 2024 08:26

Merge branch 'canary' of github.com:vercel/next.js into patch-2

58d650c

Merge branch 'canary' into patch-2

fb43141

gnoff approved these changes Mar 8, 2024

View reviewed changes

mattddean added 3 commits March 8, 2024 16:58

add test for unstable_cache undefined value retrieval

2ed5822

Merge branch 'canary' of github.com:vercel/next.js into patch-2

acab7a7

also validate that the value returned is undefined

7b5e8f0

ijjk added the area: tests label Mar 9, 2024

Merge branch 'canary' into patch-2

8ad3d50

samcx added 2 commits March 8, 2024 21:33

fix(e2e-app-dir): add new files to inline snapshot

857027c

test(e2e-app-dir): fix test

f510913

samcx merged commit 6da6388 into vercel:canary Mar 9, 2024
61 checks passed

github-actions bot added the locked label Mar 27, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly deserialize `undefined` unstable_cache data #59126

Correctly deserialize `undefined` unstable_cache data #59126

mattddean commented Nov 30, 2023 •

edited

prasad-pul left a comment

smaeda-ks Dec 10, 2023

mattddean Dec 11, 2023

mattddean Jan 22, 2024 •

edited

prasad-pul Jan 28, 2024 •

edited

stabildev commented Feb 22, 2024

prasad-pul commented Feb 22, 2024

samcx commented Feb 22, 2024

samcx commented Mar 8, 2024

mattddean commented Mar 8, 2024

samcx commented Mar 8, 2024

samcx commented Mar 8, 2024 •

edited

mattddean commented Mar 9, 2024

mattddean commented Mar 9, 2024

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited

ijjk commented Mar 9, 2024 •

edited

Facundo-Martin commented Mar 12, 2024

Correctly deserialize undefined unstable_cache data #59126

Correctly deserialize undefined unstable_cache data #59126

Conversation

mattddean commented Nov 30, 2023 • edited

prasad-pul left a comment

Choose a reason for hiding this comment

smaeda-ks Dec 10, 2023

Choose a reason for hiding this comment

mattddean Dec 11, 2023

Choose a reason for hiding this comment

mattddean Jan 22, 2024 • edited

Choose a reason for hiding this comment

prasad-pul Jan 28, 2024 • edited

Choose a reason for hiding this comment

stabildev commented Feb 22, 2024

prasad-pul commented Feb 22, 2024

samcx commented Feb 22, 2024

samcx commented Mar 8, 2024

mattddean commented Mar 8, 2024

samcx commented Mar 8, 2024

samcx commented Mar 8, 2024 • edited

mattddean commented Mar 9, 2024

mattddean commented Mar 9, 2024

ijjk commented Mar 9, 2024 • edited by samcx

ijjk commented Mar 9, 2024 • edited by samcx

ijjk commented Mar 9, 2024 • edited by samcx

ijjk commented Mar 9, 2024 • edited

Stats from current PR

ijjk commented Mar 9, 2024 • edited

Tests Passed

Facundo-Martin commented Mar 12, 2024

Correctly deserialize `undefined` unstable_cache data #59126

Correctly deserialize `undefined` unstable_cache data #59126

mattddean commented Nov 30, 2023 •

edited

mattddean Jan 22, 2024 •

edited

prasad-pul Jan 28, 2024 •

edited

samcx commented Mar 8, 2024 •

edited

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited by samcx

ijjk commented Mar 9, 2024 •

edited

ijjk commented Mar 9, 2024 •

edited