Skip to content

fix: use lazy promise for searchParams to defers execution#1901

Closed
fikrikarim wants to merge 1 commit intocanaryfrom
fikri/eng-7378-fix-500-error-on-the-search-page
Closed

fix: use lazy promise for searchParams to defers execution#1901
fikrikarim wants to merge 1 commit intocanaryfrom
fikri/eng-7378-fix-500-error-on-the-search-page

Conversation

@fikrikarim
Copy link
Copy Markdown
Member

What/Why?

Use lazy promise on searchParams to defers execution.

This is needed to fix the 500 error:

Route /[locale]/search needs to bail out of prerendering at this point because it used `await searchParams`, `searchParams.then`, or similar.

Testing

We had the same problem with cookies() on the Header: #1880

This is needed to fix 500 error:
Route /[locale]/search needs to bail out of prerendering at this point because it used `await searchParams`, `searchParams.then`, or similar.
@fikrikarim fikrikarim requested review from a team, agurtovoy and migueloller January 13, 2025 07:42
@linear
Copy link
Copy Markdown

linear Bot commented Jan 13, 2025

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 13, 2025

⚠️ No Changeset found

Latest commit: 16b48fb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 13, 2025

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

Name Status Preview Comments Updated (UTC)
catalyst-canary ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2025 7:43am
catalyst-soul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2025 7:43am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
catalyst ⬜️ Ignored (Inspect) Jan 13, 2025 7:43am
catalyst-au ⬜️ Ignored (Inspect) Jan 13, 2025 7:43am
catalyst-uk ⬜️ Ignored (Inspect) Jan 13, 2025 7:43am

Comment on lines +249 to +250
const searchParams = PLazy.from(() => pageProps.searchParams);
const props = { searchParams };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're seeing problems with searchParams similar to cookies I think that we might need a different approach here...

Having to use a lazy promise here feels wrong to me. In particular, Search should be dynamic always.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @migueloller on this. Search is being rendered dynamically though.

Copy link
Copy Markdown
Member Author

@fikrikarim fikrikarim Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search should be dynamic always.

Wait, with this change, search is still dynamic. This change just defers the searchParams execution to component that await the function.

But I agree with the general sentiment though. This is a landmine. I don't want developers to think about this quirk while developing Catalyst.

I'll close this PR for now while we search for a better solution.

@jorgemoya
Copy link
Copy Markdown
Contributor

Is this only happening when running the makeswift integration?

@migueloller
Copy link
Copy Markdown
Contributor

Is this only happening when running the makeswift integration?

Yeah. It's a race condition that happens and it's really a problem with the streamable pattern and the way React implements "postpone".

We might need to do this lazy evaluation but that might need to happen at the VIBES API layer so Catalyst isn't doing this. Or we might need to figure out a different approach.

I should probably set up a repro in the suspense-patterns repo so we can explore ideas

@fikrikarim fikrikarim closed this Jan 14, 2025
@fikrikarim
Copy link
Copy Markdown
Member Author

Is this only happening when running the makeswift integration?

No. This happens on the canary branch too.

image (21)

But this happens more often on the integrations branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants