Handle bigint in useQuerySubscription

[rkofman]

fixes #4279

Handles query arg values that include bigints while retaining behavior of always initiating refetch (even when user-supplied serializeQueryArgs doesn't change). This is an alternative proposed solution from #4297 -- which fixes bigint errors but diverges refetching behavior.

Note: this changes behavior for all callers of defaultSerializeQueryArgs; which should be safe if the library doesn't try to re-serialize the args anywhere. It may result in slightly surprising behavior for consumers (a potentially unexpected default serialization of bigint values) - but since it currently breaks hard with an exception for those use cases, unlikely to be a breaking change for anybody.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e22da96:

Sandbox	Source
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration
rtk-esm-cra	Configuration

[netlify]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	e22da96
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/66140fc723051a000899eebf
😎 Deploy Preview	https://deploy-preview-4315--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[rkofman]

@riqts thoughts on this PR? Is there anything process-wise I need to do before I get a review? e.g. I'm happy to finish implementing additional unit tests.

[riqts]

Haha, I think unfortunately just nothing moves fast in volunteer work. Realistically the primary people who are fully tuned in to this area of the codebase are @phryneas or @markerikson as they wrote the code here and are most familiar with its intentions.

But unfortunately I think they have been mega busy and there is a decent amount of cognitive load they have to take on for this particular fix. The value vs effort for this particular fix probs just doesn't feel great with the workload.

I will try add some context in the issue of the surrounding code so it's easier for the reviewers to engage with the PRs.

Potentially this PR itself might be preferred if instead of exposing a replacer it just handled potential incoming bigints within the defaultSerializeQueryArgs and the implementation of the exposed option can just go in a seperate issue to get it moved along?

[markerikson]

Yeah, to be frank, I've been otherwise occupied with personal stuff for much of this year and haven't had mental bandwidth to do meaningful RTK work (either myself or reviewing actual code PRs).

I've seen this PR, I'm very aware of it, but can't give an immediate timeline when I'll have the bandwidth to think through this one.

[rkofman]

[non-technical aside]
♥️ to both of you! I have little idea what it takes to maintain a project of this scope / popularity and am very glad that there are people who do. I suspect my can I be more helpful? pings may have landed as more demanding of immediate attention than intended. To be clear: I am not blocked in my project (and tbh, a full half of the places we use bigints today -- we really shouldn't be, but that's another story).

The major pressures I'm feeling are: (1) I just don't know what the expectations are when contributing (how much I should carry water for the change myself, how much to wait for helpful context from others, etc.) and (2) I'm a little afraid that I will drop context over time, and would feel disappointed (at myself) for not helping to finalize and land this change if it pages out of my working memory. That sense of urgency is a matter of personal convenience and takes a back seat to maintainer needs.
[/aside]

@riqts happy to create an alternative PR that handles bigint directly in defaultSerializeQueryArgs. The reason I shied away from it in the first place is I have a difficult time reasoning through all the places defaultSerializeQueryArgs is used; and what impact serializing previously unserializable data would have. For example, are there places in the library that expect to deserialize it? If so, those need to be updated with a mirror implementation. I would probably want to take a few days (up to a week) to poke around and see if I can get mysefl to understand the impact before opening that PR.

[rkofman]

One more idea would be to just re-implement defaultSerializeQueryArgs in useQuery. This would:

1. remove the public interface change
2. maintain correct behavior
3. limit scope of serialization change to avoid regressions elsewhere

The main downside is code duplication -- but that should be easy to cleanup later for somebody with more context (it punts the decision of spreading the change everywhere vs. introducing a configuration parameter).

[EskiMojo14]

from memory I don't think we deserialize the results of serializeQueryArgs - it's only used to compare equality of two values (for example, creating a cache key)

we keep hold of the original value for things like refetching so there's no need to deserialize

[rkofman]

As suggested; updated to the simplest thing that can work (and added unit test for defaultSerializeQueryArgs).

[rkofman]

(.. and updated the PR description to match the new code ...)

[markerikson]

Okay, finally swinging back to this (had an evening where I had both time and mental energy to chew through a bunch of open PRs). Thanks for the work here, and for putting up with the delay!