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

Concurrent Mode and UseSubscription with RxJS "lose" updates #17314

Closed
samcooke98 opened this issue Nov 8, 2019 · 9 comments
Closed

Concurrent Mode and UseSubscription with RxJS "lose" updates #17314

samcooke98 opened this issue Nov 8, 2019 · 9 comments

Comments

@samcooke98
Copy link

samcooke98 commented Nov 8, 2019

Do you want to request a feature or report a bug?
Bug - I think?

What is the current behavior?

In Concurrent Mode, it appears that if a render is interrupted, if a component is using useSubscription the interrupted update is lost, which leads to "tearing"

The following codesandbox uses useSubscription with a RxJS BehaviorSubject, mimicking the example from here: https://www.npmjs.com/package/use-subscription#subscribing-to-observables

In the sandbox, clicking on the "Increment Remote Count" button triggers the RxJs BehaviorSubject to increment. This is done outside of the React event handler (ie: via window.addEventLIstener and so the updates are not batched together. The update to render the numbers is artificially slowed down.

If you click the "Increment Remote Count" button multiple times, the update works as expected.

If you interrupt the update, via clicking the "increment local count", only the last number will update.

So the Steps to reproduce look like:

  1. Click the "Increment Remote Count" button once
  2. Before the update is committed to the DOM, click the "Increment Local Count" update.
  3. The first update is "lost" ie; the output looks like:

image

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://jwenc.csb.app/ \ https://codesandbox.io/s/usesubscriptionconcurrentlosingupdates-jwenc

What is the expected behavior?

I'd expect there to be a commit as the above screenshot, but I'd then expect there to be a follow-up commit that restores the consistency. In other words, I'd expect in the above picture for everything to be 1

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

0.0.0-experimental-f6b8d31a7

I'd be willing to try to take a stab at writing a React test for this, if needed?

@dai-shi
Copy link
Contributor

dai-shi commented Nov 8, 2019

#16396 (comment)
Here's codesandbox I created.

@samcooke98
Copy link
Author

Started looking into why this happens - #17336

Seems like an issue somewhere in updateSimpleMemoComponent

@dai-shi
Copy link
Contributor

dai-shi commented Nov 14, 2019

Maybe related: #17318 (comment)

@salvoravida
Copy link

yes i already signaled here : #17318 (comment)

@albertogasparin
Copy link

Reported similar issue some time ago: #17028

@acdlite
Copy link
Collaborator

acdlite commented Feb 21, 2020

Thanks for the bug report!

I believe this was fixed by #18091

You can confirm using react@0.0.0-experimental-ea6ed3dbb and react-dom@0.0.0-experimental-ea6ed3dbb.

I'm going to close this issue, but if the bug persists please comment and we'll reopen.

@acdlite acdlite closed this as completed Feb 21, 2020
@samcooke98
Copy link
Author

Rebased https://github.com/facebook/react/pull/17336/files and the test was green - thanks!

@acdlite
Copy link
Collaborator

acdlite commented Feb 21, 2020

@samcooke98 Thank you for that PR! I feel a bit guilty that it got buried and I didn't notice it. But I really appreciate that you took the time to write a unit test 😮We'll try to do a better job keeping track of our PR queue!

@samcooke98
Copy link
Author

No worries, was a nice learning experience

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

No branches or pull requests

6 participants