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

React Router - Concurrent updates fail #54

Closed
geemanjs opened this issue Oct 18, 2019 · 4 comments · Fixed by #61
Closed

React Router - Concurrent updates fail #54

geemanjs opened this issue Oct 18, 2019 · 4 comments · Fixed by #61

Comments

@geemanjs
Copy link

geemanjs commented Oct 18, 2019

Hey,

Thanks for the library I've been using it extensively!

Versions:

use-query-params: 0.4.2
react-router: 5.1.2
react-router-dom: 5.1.2

When calling two setQuery functions from the same "event" only one is updated.

See this video
output_optimized

Reproducable code sample
https://codesandbox.io/embed/lucid-pike-g5vok

The usecase here is noddy but the implementation is the important part. In my use case I have two very different abstractions which is why they have seperate hooks and don't use useQueryParams instead

I believe it will be an issue with the way use-query-param uses react-routers Route component

I wanted to highlight the issue before investigating

@pbeshai
Copy link
Owner

pbeshai commented Oct 18, 2019

Hmm, yea that's annoying one solution is #53 at first glance. But maybe there's another where we can re-read the URL just before updating to ensure we have the latest value.

@geemanjs
Copy link
Author

Reading through I'm not sure how batching would work with this.. happy to help implement something if you have an idea.

The problem spans from the fact they both use the location they were given at the time the component was rendered.

For anyone that comes here looking for an immediate workaround - you can wrap the call in a setTimeout so the update falls out of the current callstack. But this is entirely relying on a side effect that they point to the same 'location' instance which is far from best practice.

setTimeout(() => { selectX(e.nativeEvent.x) }, 0)

@pbeshai
Copy link
Owner

pbeshai commented Oct 18, 2019

The problem spans from the fact they both use the location they were given at the time the component was rendered.

Right, this is why I suggested we can re-read the URL just before updating to ensure we have the latest value. Batching would avoid this problem since it would combine the edits into a single edit to do at the same time. But doing batching requires making useQueryParams asynchronous and is probably a substantial change.

Unfortunately the only other ideas I have are to (a) read from window.location instead of the location in context or (b) have the context provide a function that returns the current location value instead of giving the value directly. (b) could work with a default implementation using window.location and maybe with React Router we could look at history.location. Not sure without further investigation.

@pbeshai
Copy link
Owner

pbeshai commented Nov 16, 2019

In v0.4.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants