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

Broken in next@14.1.0 #481

Closed
ianldgs opened this issue Jan 31, 2024 · 12 comments · Fixed by #482
Closed

Broken in next@14.1.0 #481

ianldgs opened this issue Jan 31, 2024 · 12 comments · Fixed by #482
Labels

Comments

@ianldgs
Copy link

ianldgs commented Jan 31, 2024

Context

What's your version of nuqs?
1.15.4 (current latest)

Are you using:

  • ✅ The app router
  • ❌ The pages router
  • ❌ The basePath option in your Next.js config
  • ✅ The experimental windowHistorySupport flag in your Next.js config

Description

In next 14.1.0, windowHistorySupport is enabled by default:
vercel/next.js#60557

And according to the docs, the call to history.pushState or history.replaceState has to be with state = null, like so:
window.history.pushState(null, '', params.toString()) / window.history.pushState(null, '', params.toString())

Current version passes state = history.state:

updateMethod.call(
history,
history.state,
// Our own updates have a marker to prevent syncing
// when the URL changes (we've already sync'd them up
// via `emitter.emit(key, newValue)` above, without
// going through the parsers).
NOSYNC_MARKER,
url
)

That causes any useSearchParams to not be reactive to calls made via nuqs.

Side node:
In my company we have a similar package to nuqs; I've tested passing state = null and it worked for us.
One thing that made the migration painful was that nuqs doesn't handle URLSearchParam native arrays (.append and .getAll). So we ended up deciding to keep our current solution, as it was easier to just fix it.

@ianldgs ianldgs added the bug Something isn't working label Jan 31, 2024
@ianldgs ianldgs changed the title Broken in next@14.1 Broken in next@14.1.0 Jan 31, 2024
@franky47
Copy link
Member

Thanks for your report. I'm not sure what you mean by "broken", since all tests pass on 14.1.0 (and the docs webapp also runs 14.1.0 and works fine).

nuqs doesn't handle URLSearchParam native arrays (.append and .getAll)

This is indeed one limitation that I would like to address in a future release, which may require a big rewrite of the parsers. If you have some feedback on a good API for this, I'd love to discuss more about it.

@ianldgs
Copy link
Author

ianldgs commented Jan 31, 2024

Sorry, I realized I wasn't clear at first and edited the description 🙈
But it's not working as expected if used with useSearchParams.
Not sure if any tests cover that. I would probably require an e2e test to verify it.

@ianldgs

This comment was marked as off-topic.

@franky47
Copy link
Member

franky47 commented Jan 31, 2024

But it's not working as expected if used with useSearchParams.

Could it be that the internal state is updated immediately, and useSearchParams lags behind when the URL is actually updated, because of the throttle/defer mechanism?

It's a genuine source of issues that I need to address in the docs.

I could add an e2e test for this, useSearchParams should be eventually consistent with the query state, when the promise returned by setState resolves.

@franky47
Copy link
Member

franky47 commented Jan 31, 2024

I ran the test suite with setting the shallow history state to null as recommended in the Next.js docs, and everything seems to run smoothly. I'll run more tests against useSearchParams and will keep you up to date.

The initial idea in maintaining the history state was to be as transparent as possible while Next.js didn't have proper shallow routing support (ie: maintaining the internal router state in the history state). I'll have to run some more tests of the impact of setting a null state on older versions of Next.js.

Edit: as expected, this breaks with older versions of Next.js: https://github.com/47ng/nuqs/actions/runs/7731026896

@aldenquimby
Copy link

Noting that I tried to migrate to nuqs recently from a home grown frankenstein, and ran into this bug, so I couldn't proceed. On Next 14.1.0 with latest nuqs, my useSearchParams consumers are not reacting/triggering to query string changes made via nuqs

@skve
Copy link

skve commented Jan 31, 2024

Yep, same issue here, unfortunately renders it pretty useless for me. @franky47 happy to help you if I can, really appreciate the effort in this lib over the past few days.

@franky47
Copy link
Member

franky47 commented Feb 1, 2024

Thank you all for the feedback, it's been very helpful.

I have a fix in the pipeline that maintains compatibility with older versions of Next.js. I'm not a big fan of doing version comparisons in lib code, so this will probably mean that v2.0.0 will only support next@^14.1.0.

Copy link

github-actions bot commented Feb 1, 2024

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

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Feb 1, 2024

🎉 This issue has been resolved in version 1.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ianldgs
Copy link
Author

ianldgs commented Feb 1, 2024

The initial idea in maintaining the history state was to be as transparent as possible while Next.js didn't have proper shallow routing support

Yeah. In our library, before updating to next 14.1.0 enabled windowHistorySupport by default, it was also working with maintaining the state. Updating next broke it.

that 2.0.0 will only support next@^14.1.0.

Sounds like a good decision!

Nice work on fixing this so fast @franky47 👍

@aldenquimby
Copy link

Thank you @franky47 !

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.

4 participants