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

fix: Push #419

Merged
merged 11 commits into from
Dec 10, 2023
Merged

fix: Push #419

merged 11 commits into from
Dec 10, 2023

Conversation

franky47
Copy link
Member

@franky47 franky47 commented Dec 2, 2023

History: push is currently broken on Next.js 14.0.3 in the app router without the WHS flag, and has been broken for a while in the pages router.

Adding tests to cover those cases, and looking for ways to solve it, though in the case of the app router it may depend on vercel/next.js#58861 to be made available.

Getting a consistent fix in the pages router that doen't break dynamic routes or basePath support is tricky. Initially I thought it was pointless to go through the History API in the pages router, as it supports shallow routing, but always going through the router without extra care seems to break in some combinations of settings and situations.

Copy link

vercel bot commented Dec 2, 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 Dec 10, 2023 2:27pm

@franky47
Copy link
Member Author

franky47 commented Dec 6, 2023

Still some issues with basePath on the pages router, where basePath is applied twice. Relevant Next.js issues:

Edit: renaming the failing test folder seems to "fix" the issue, but some other names also yield this error (eg: useSomethingElse rather than useQueryState for the directory name).

Definitely won't bother chasing non-deterministic bloom filter errors, so I'll just reorder the test directories to avoid clashes.

Going through the history API only on shallow updates doesn't
notify the pages router, which can handle shallow updates directly.

Definitely a hack, but it allows also setting the asPath for dynamic
routes. There is still an issue with basePath being applied twice,
but that's an internal Next.js bug for another day.
Disabling the bloom filter causing invalid collisions
between the pages and app routers.
@franky47 franky47 marked this pull request as ready for review December 10, 2023 14:11
@franky47 franky47 merged commit 54c1328 into next Dec 10, 2023
13 checks passed
@franky47 franky47 deleted the fix/push-on-pages-router branch December 10, 2023 14:30
Copy link

🎉 This PR is included in version 1.13.2-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 1.13.2 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant