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.

Sign up for GitHub

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

Jump to bottom

Fix 500 error (needs to bail out of prerendering at this point because it used cookies) #1850

Closed
wants to merge 1 commit into from
Closed

Fix 500 error (needs to bail out of prerendering at this point because it used cookies) #1850

wants to merge 1 commit into from

Conversation

fikrikarim
Copy link
Member

@fikrikarim fikrikarim commented Jan 6, 2025 •
edited
Loading

What/Why?

Fix 500 cookies() error

Unhandled Rejection: Error: Route /[locale]/[...rest] needs to bail out of prerendering at this point because it used cookies

Testing

Here are the screenshot logs of the integration branch vs the PR branch when I open the /home and /shop-all.

Integration branch:
image

PR branch:
image

@fikrikarim fikrikarim requested a review from a team as a code owner January 6, 2025 16:01
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 857bd7a

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 Vercel
Copy link

vercel bot commented Jan 6, 2025 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
catalyst-latest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 11:36pm
catalyst-soul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 11:36pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
catalyst ⬜️ Ignored (Inspect) Jan 6, 2025 11:36pm
catalyst-au ⬜️ Ignored (Inspect) Visit Preview Jan 6, 2025 11:36pm
catalyst-uk ⬜️ Ignored (Inspect) Visit Preview Jan 6, 2025 11:36pm

agurtovoy
agurtovoy approved these changes Jan 6, 2025
View reviewed changes
Copy link

@agurtovoy agurtovoy left a comment

Choose a reason for hiding this comment

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

🙏

migueloller
migueloller reviewed Jan 6, 2025
View reviewed changes
core/components/header/index.tsx
Comment on lines +94 to +95
const cookieStore = await cookies();
const cartId = cookieStore.get('cartId')?.value ?? null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note, the reason we did this was to avoid the header from being dynamic for non-logged in users, thus resulting in worse performance. With PPR, we should be able to have just the cart count component be dynamic.

Does this change make every page that uses Header (i.e., all pages) dynamic? Have we checked impact on performance?

Copy link

Choose a reason for hiding this comment

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

I'd also note that PPR is experimental and "not ready for production use".

@fikrikarim fikrikarim marked this pull request as draft January 6, 2025 18:36
@fikrikarim
Copy link
Member Author

Marking this as a draft as we investigate the issue further.

@migueloller migueloller marked this pull request as ready for review January 6, 2025 23:33
jorgemoya
jorgemoya requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@jorgemoya jorgemoya left a comment

Choose a reason for hiding this comment

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

I think if this is only happening in the Makeswift integration branch, apply the change on that branch until we can figure it out.

@agurtovoy agurtovoy mentioned this pull request Jan 6, 2025
Merged
@agurtovoy
Copy link

Cherry-picked and merged to soul/integrations/makeswift: #1856

@migueloller migueloller closed this Jan 7, 2025
@fikrikarim
Copy link
Member Author

Love love love @agurtovoy ❤️

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

@agurtovoy agurtovoy agurtovoy approved these changes

@jorgemoya jorgemoya jorgemoya requested changes

@migueloller migueloller migueloller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fikrikarim @agurtovoy @jorgemoya @migueloller