Skip to content
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

feat: Add search params cache #397

Merged
merged 17 commits into from
Nov 23, 2023
Merged

feat: Add search params cache #397

merged 17 commits into from
Nov 23, 2023

Conversation

franky47
Copy link
Member

@franky47 franky47 commented Nov 16, 2023

This simplifies the use of parsers on the server, and allows accessing search params in a type-safe manner in deeply nested server components.

Big thanks to @eric-burel for the tip on how to use React's cache this way.

  • Test that cache is indeed empty in layouts (NUQS-500) -> It's not, if the whole page is switched to dynamic rendering
  • Ignore react from size-limit on server side code

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-usequerystate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 11:08pm

@franky47 franky47 mentioned this pull request Nov 16, 2023
@franky47 franky47 force-pushed the feat/search-params-cache branch from 9e7ade4 to 86a323d Compare November 16, 2023 09:00
@franky47 franky47 force-pushed the feat/search-params-cache branch from bb00080 to 33f32e8 Compare November 16, 2023 12:52
@franky47 franky47 force-pushed the feat/search-params-cache branch from 33f32e8 to 09330cf Compare November 16, 2023 22:34
@franky47 franky47 force-pushed the feat/search-params-cache branch from 7d7106f to 74543f4 Compare November 17, 2023 01:10
@franky47 franky47 force-pushed the feat/search-params-cache branch from 74543f4 to c5318ae Compare November 17, 2023 01:11
@franky47 franky47 force-pushed the feat/search-params-cache branch from c5318ae to 1f4a44a Compare November 21, 2023 02:51
@franky47
Copy link
Member Author

franky47 commented Nov 21, 2023

This API does not make much sense, when the useQueryState(s) hooks could be used server-side as a read-only interface for type-safe search params, leveraging the useSearchParams hook, which brings the same behaviour as using the searchParams props at the top of the page, but without the need to parse anything at the top level.

Edit: it still makes sense. useSearchParams only works in Client Components, so if a deeply nested RSC needs access to search params, there's no current way to do that in stock Next.js.

@Talent30
Copy link
Contributor

This API does not make much sense, when the useQueryState(s) hooks could be used server-side as a read-only interface for type-safe search params, leveraging the useSearchParams hook, which brings the same behaviour as using the searchParams props at the top of the page, but without the need to parse anything at the top level.

Edit: it still makes sense. useSearchParams only works in Client Components, so if a deeply nested RSC needs access to search params, there's no current way to do that in stock Next.js.

Yes, It definitely has value in terms of preventing prop drilling in server components. Good stuff! I have learned something new from this PR.

@franky47
Copy link
Member Author

franky47 commented Nov 23, 2023

CI is failing with the weirdest bug..

Changing the cache test case by adding two children pages /app/cache/a and /app/cache/b, and have them <Link> to each other, breaks the pages router non-shallow updates, but only when there's a basePath present, and only in Next.js 13.5 and after 🙃

Edit: links are not at fault, it's just the mere presence of the second child page. Even if both of them contains a simple static Hello world.

Starting to question my sanity here.

@Talent30
Copy link
Contributor

CI is failing with the weirdest bug..

Changing the cache test case by adding two children pages /app/cache/a and /app/cache/b, and have them <Link> to each other, breaks the pages router non-shallow updates, but only when there's a basePath present, and only in Next.js 13.5 and after 🙃

Edit: no, it's just the mere presence of the second child page. Even if both of them contains a simple static Hello world.

Starting to question my sanity here.

How about when you are using router.push to go to page b?

@Talent30
Copy link
Contributor

@Talent30 that's the thing, I'm not even navigating to those pages!! Their mere presence in the build causes the pages router (a completely different subsection of the app) to fail in weird ways. I feel like this is a Next.js bug which has been dormant until then, and somehow this very specific combination of setup causes it.

The effect is that updates in the pages router don't apply the basePath correctly, and end up shallow-navigating to a 404.

It's so weird I'm considering just dropping those test pages just to restore CI and move on with the cache feature. Maybe our paths will cross again, and I will be ready ⚔️ . Until then, 🤷‍♂️

Ah sorry I missed the part. Yea I don't think we should worry about the page router since it is not designed for RSC(cache is part of the RSC proposal) and I don't think Next.js team would fix it even you report it.

@franky47
Copy link
Member Author

Actually it made me realise the e2e test did not properly catch the underlying issue: basePath support is broken on the pages router at the moment, and the issue above only uncovered the problem in another way. But running the e2e test bench shows the basePath not being properly handled.

I'll rework those tests in a separate PR.

As for the cache, I think it's good to go, there should be an error thrown at build time if it's being used in a Layout (or any component under a layout), to highlight the fact that even though Layouts may receive searchParams via the cache feature on page reload, it goes stale instantly on client-side navigation, so it should not be trusted.

franky47 added a commit that referenced this pull request Nov 23, 2023
What the actual freaking fuck?
The mere presence of those pages (in the app router),
and even if they only were to contain a static Hello world
output, would cause the **pages** router to fail to apply
updates, when there is a basePath, and on Next.js 13.5+.

Losing sanity here trying to figure out what's wrong,
and it's the middle of the night so I won't go deeper
into this right now.

See #397 (comment)
@franky47 franky47 force-pushed the feat/search-params-cache branch from b45b16a to 7c2ef46 Compare November 23, 2023 21:58
This simplifies the use of parsers on the server, and allows
accessing search params in a type-safe manner in deeply
nested server components.

Big thanks to @eric-burel for the tip on how to use React's `cache` this way.
Running out of relevant HTTP status codes.. Maybe this was a bad idea.
This allows accessing the parsed values straight from the
page component. Freezing also ensures the object reference
stays immutable for the whole duration of the page render.
Cache can't be exported from the page file,
so instead showcase the "soft convention"
of using a searchParams.ts file next to the page.
What the actual freaking fuck?
The mere presence of those pages (in the app router),
and even if they only were to contain a static Hello world
output, would cause the **pages** router to fail to apply
updates, when there is a basePath, and on Next.js 13.5+.

Losing sanity here trying to figure out what's wrong,
and it's the middle of the night so I won't go deeper
into this right now.

See #397 (comment)
@franky47 franky47 merged commit d4bf14e into next Nov 23, 2023
12 checks passed
@franky47 franky47 deleted the feat/search-params-cache branch November 23, 2023 23:12
Copy link

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants