dai-shi / will-this-react-global-state-work-in-concurrent-rendering

Test tearing and branching in React concurrent rendering
MIT License
558 stars 45 forks source link

Questions on Tests #47

Closed selsamman closed 2 years ago

selsamman commented 3 years ago

Thanks so much for this project. I am working on getting a new yet-to-be formally published mutable state library tested in concurrent mode. Adding it to this project produced failures on test 4 and 6 which is not surprising. I do want to make sure I truly understand, the tests themselves and want to make sure these assumptions are correct:

  1. Test 2 is just making sure that after a transition when rendering sub-components that get shared state and are slow rendering that the screen will never show a mix of the old and new values for these components as does Dan's subscription example in the reactwg/react-18
  2. Test 4 goes further and ensures that while the transition is pending that the original results stay on the screen.
  3. Test 6 try's to demonstrate tearing due to changes in state that come in from user events during a series of related renders.

I also curious as to why the redux example with useMutableSource fails on test 4 & 6. Is this because the one that is being used (unstable_useMutableSource) is not final?

dai-shi commented 3 years ago

You get Test 2 right.

Test 5 and 6 (corresponding to Test 1 and 2 respectively) are a trial to distinguish userland solution like useSubscription and native solution like unstable_useMutableSource. It used to work better in some experimental versions of React, but now it behaves weirdly and can't explain well. At least the pure useState passes those two tests.

Test 4 is for "branching". If branching works like useState, the update during pending is queued with priority, and flushed in that order. It shows "6" before "10".

Redux has a store externally, and branching doesn't work at the moment.

selsamman commented 3 years ago

I had planned to use useMutableSource to manage a state version number and then branch accordingly within my proxy. After reading your response on useMutableSource not yet being ready I have been experimenting with the alternative of just keeping the state version number in useState and using that to drive the internal branching which shows promise. I do think that proper support of branching (showing the previous state) is important in state libraries and maybe even more so in transitions that involve long delays like fetching data (e.g the Beatles examples in the WG). Thanks again for your insight and especially for the set of tests in this project.

dai-shi commented 3 years ago

Please note that uMS will be replaced with uSES. https://github.com/reactwg/react-18/discussions/86

(showing the previous state)

What Test 4 does is more than this. It's more or less nitpicking at this point.

selsamman commented 3 years ago

Things are changing quickly on the R18 front indeed. I am making good progress on using useState in a context just to track a transition version number and then delivering the corresponding snapshot from within my proxy. All 6 tests are passing with my local version so there is hope that a mutable state manager could pass all tests without using uSES. Hope to have the full proof published soon.

On the subject of uSES, When Andrew says "Updates triggered by a store change will always be synchronous, even when wrapped in startTransition" I assume that means that when an external state manager, using uSES, triggers an expensive render, the app would not be responsive to events until the entire synchronous render was complete.

So I am wondering if test 3 would start failing on state managers that use uSES.

selsamman commented 3 years ago

I am throwing in the towel on making test 4 & 6 working. Too much overhead and complexity to commit it. Will wait to see uSES.

You are right that test 4 is somewhat nitpicking but it does represent a difference that useState/useContext based mechanisms offer. Mutable state managers tend not to defer updates but rather offer snapshots for the un-transitioned state. I assume that is what Valtio is doing with useSnapshot and that is what I am doing as well by taking snapshots during transitions. So to offer an un-transitioned state to an event (doubleIncrement) which is how it would get from 3 to 6 seemed a bridge too far in the end.

However one other branching related issue that the tests do not currently address is ensuring what is rendered while isPending is true is the original and not the transitioned state. This is not as much of a nit as long transitions that depend on suspense to complete may be displaying the old state for a visible amount of time.

To that end I propose that test 2 should ensure that the count obtained from useCount remains unchanged. You can see that change here in a branch of my fork of your project. It does make a couple libraries fail test 2. It just seems to me that a test described as "updated properly with transition" should include the period of the transition as well. Alternatively another test could be added.

Also I have to say this project really helped me to give Proxily a shakeout with React 18 (at least in its current form). I contributed an update script to run the tests and automatically update the README.md

Let me know if you want me to file PRs for any of this.

dai-shi commented 3 years ago

the tests do not currently address is ensuring what is rendered while isPending is true is the original and not the transitioned state.

Great catch. It's a nice idea to add this. Isn't it test 1 instead of test 2? Feel free to file a PR and we discuss there.

an update script to run the tests and automatically update the README.md

Can you do it for this project? A PR would be nice. ref: https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering/issues/44#issuecomment-870352907

selsamman commented 3 years ago

Yes sorry it is test 1 that I modified to add this. I should mention that there are also some minor changes to common to default-parameterize items for proxy-based state management libraries like mobx, Proxily, react-easy-state which use a wrapper other than React.memo. Will file separate PRs for each topic so discussion can be tracked for each.

dai-shi commented 3 years ago

Will file separate PRs for each topic so discussion can be tracked for each.

That sounds great.

dai-shi commented 2 years ago

Improved with #56, closing for now. Feel free to continue discussion.