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

Demo with dynamic routing ? #282

Closed
spicq-rambobafet opened this issue Feb 2, 2022 · 17 comments · Fixed by #391
Closed

Demo with dynamic routing ? #282

spicq-rambobafet opened this issue Feb 2, 2022 · 17 comments · Fixed by #391
Labels

Comments

@spicq-rambobafet
Copy link

Hello,

I'm currently having an issue with useQueryState when used in complement of a dynamic routing. I might be missing something but couldn't find information in the doc :x

I've got something like : 'pages/product/[slug]/[[...params]]' and I'd like to keep some detail about my product in the URL (variant, size...) but as soon as I set a value I get this error :

Unhandled Runtime Error
Error: The provided href (/product/[slug]/[[...params]]?variant=od) value is missing query values (slug, params) to be interpolated properly. Read more: https://nextjs.org/docs/messages/href-interpolation-failed

I use the version 1.6.0 of next-useQueryState.

Any ideas?

Thanks,
Bobu

@franky47
Copy link
Member

franky47 commented Feb 2, 2022

Thanks for the report, that's a bug indeed.

It looks like we're missing the query part of the router when calling router.push|replace, which does this interpolation of dynamic URL params. Though we're using the asPath which is fully interpolated already 🤔

Ideally, we should only touch the querystring (the search property in the router arguments) and leave the rest as-is, but this could require having the router as a dependency in the useCallback for the state update function, which would make it lose its referential stability.

Would you like to open a PR?

@franky47 franky47 added the bug Something isn't working label Feb 2, 2022
franky47 added a commit that referenced this issue Feb 2, 2022
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

🎉 This issue has been resolved in version 1.6.1-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47
Copy link
Member

franky47 commented Feb 2, 2022

I've published a fix attempt in 1.6.1-beta.1, could you give it a try and tell me if it solves your problem?

The drawback is that now the state updater function will be recreated when router.query changes, but it should not be much of an issue as it usually changes during page transitions, where contents & hooks are unmounted/remounted anyway.

@spicq-rambobafet
Copy link
Author

Hi! Thanks for the quick reply and patch!
Looks good to me, no more errors 🎉

@franky47 franky47 reopened this Feb 4, 2022
@franky47
Copy link
Member

franky47 commented Feb 4, 2022

Hum this fix had a side-effect of no longer allowing to clear the query by setting the state to null, I'll look into that before publishing a release.

Also my initial assumption was flawed: router.query contains both the querystring (URL.search) and the URL parameters in dynamic routes. There's no winning here, the state updater function will have to change on each query change 😭

Edit: no longer using router.query, but extracting the resolved pathname from router.asPath seems to do the trick. Which makes sense because it's actually used as such in the state updater function (where is eslint when I need it 😅).
Using the whole asPath would break referential integrity as it also contains the querystring.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

🎉 This issue has been resolved in version 1.7.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

🎉 This issue has been resolved in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47
Copy link
Member

franky47 commented Feb 4, 2022

Closing as I've got E2E tests that cover this case now. Let me know if you still encounter some issues!

@franky47 franky47 closed this as completed Feb 4, 2022
@franky47
Copy link
Member

franky47 commented Feb 7, 2022

Reopening as I'm now encountering this issue (but not in E2E tests, maybe not all cases are covered).

We could theoretically use window.location.pathname as a replacement for the asPath here, if we assume that the query updater function should only work on the query string (ie: that the pathname remains the same before and after setState).

@SimonProper
Copy link

Do we have any updates on this issue? Ran into the same problem as above today when using the hook inside a dynamic route using the pages directory.
Sadly to green on this project to open a PR at the moment, but happy to help if I get some directions on where to start!

@franky47
Copy link
Member

franky47 commented Nov 9, 2023

@SimonProper what version of Next.js and next-usequerystate are you using? Are you able to update to 1.8+ (it requires Next.js 13.4+)? The implementation has changed a lot since this issue was (re)opened, and tests with dynamic routes in the pages router seem promising (there is no longer need for the asPath when using next/navigation).

@SimonProper
Copy link

@franky47 I did indeed have the wrong version of next, however the problem seems to persists even after updating to next v.14.0.1 and next-usequerystate 1.10.0. I will see if I can make a minimal reproduction example

@SimonProper
Copy link

@franky47 So I managed to reproduce the issue. It is occurring when you are setting a value with the option shallow:false inside of a dynamic page.

Have a look at the codesanbox bellow.

  1. click the link to the dynamic route
  2. click the button

https://codesandbox.io/p/sandbox/next-usequerystate-dynamic-page-issue-79zgvw

franky47 added a commit that referenced this issue Nov 10, 2023
franky47 added a commit that referenced this issue Nov 10, 2023
* fix: Non-shallow updates on dynamic pages

Closes #282.

* chore: Fix undefined when running test locally

* chore: Fix Turbo dependency on basePath

* fix: Handle base path
Copy link

🎉 This issue has been resolved in version 1.10.2-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47
Copy link
Member

@SimonProper could you try 1.10.2-beta.1 and tell me if it works out for you?

@SimonProper
Copy link

@franky47 I can confirm that it works with your latest changes in 1.10.2-beta-1. Nice work!
Forked the reproduction and updated the version if you want to test out for yourself:
https://codesandbox.io/p/sandbox/next-usequerystate-dynamic-page-issue-1-10-2-beta-1-vmmfp2

Copy link

🎉 This issue has been resolved in version 1.10.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
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants