fix CM tearing #1455

salvoravida · 2019-11-08T12:27:48Z

Hi folks.
My idea is to change forceRender into useSelector hook, let instead set a state, and return that state.
This should transform redux state changes into react useState changes that should batched and eventually paused form react concurrent mode

netlify · 2019-11-08T12:29:06Z

Deploy preview for react-redux-docs ready!

Built with commit 16a6253

https://deploy-preview-1455--react-redux-docs.netlify.com

salvoravida · 2019-11-08T12:34:22Z

this should fix this demo
#1351 (comment)

dai-shi · 2019-11-08T13:14:11Z

This is probably a similar approach to my old implementation of reactive-react-redux.

It is important to keep consistency during intermediate updates, so you want to keep the test unchanged.
I think it's doable if you take the same approach as use-subscription.

It would actually be interesting to develop user-subscription based react-redux,
although I'm not motivated because I would like to push my approach.
I think the redux team is waiting for the core support, so they might not be so interested with the approach with use-subscription. But, anyway forking and experimenting should be good.

salvoravida · 2019-11-08T13:27:23Z

finally it fix even useTransition and isPending

https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-pytbh

salvoravida · 2019-11-08T13:30:28Z

This is probably a similar approach to my old implementation of reactive-react-redux.

It is important to keep consistency during intermediate updates, so you want to keep the test unchanged.
I think it's doable if you take the same approach as use-subscription.

It would actually be interesting to develop user-subscription based react-redux,
although I'm not motivated because I would like to push my approach.
I think the redux team is waiting for the core support, so they might not be so interested with the approach with use-subscription. But, anyway forking and experimenting should be good.

the only test that i fix, is the one where the parent force the same value to the child, that need 2 render to CORRECTLY process all store updates. reading store on render is an hack to have 1 render less on that "rarely usecase" but that break the normal queque from Redux to useState.

eddyw · 2019-11-08T13:43:36Z

See #1351 (comment)
If component is wrapped within React.memo, it doesn't work 😛

eddyw · 2019-11-08T13:59:08Z

src/hooks/useSelector.js

@@ -31,7 +31,7 @@ function useSelectorWithStoreAndSubscription(
      selector !== latestSelector.current ||
      latestSubscriptionCallbackError.current


To be CM friendly, could we do something like:

try { ... } catch(e) { if (typeof e === 'object' && e !== null && typeof e.then === 'function') throw e let errorMessage = ...

So we could do this, for example:

const status = useSelector(s => { if (s.status === STATUS.PENDING) throw promise; return s.status; }) }

In React docs, it's using something like:
const value = resource.read()
where read() either returns the value or throws so it can bail out before the selector is set. Not sure if here is the best place to do it, but yeah, with CM not everything "thrown" is an error.

sorry i think that thow promise to comunicate with Suspense does NOT matter with useSelector, that has to select a state without tearing on render. that's another problem.

salvoravida · 2019-11-08T16:55:27Z

See #1351 (comment)
If component is wrapped within React.memo, it doesn't work 😛

here is the fix 😛😛😛 https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-salvoravidareact-redux-9jo9z

but please note that is a bug of the current "experimental react" with "SimpleMemoComponent"
infact force memo Non simple it works.

react-dom-internal

function updateMemoComponent(current$$1, workInProgress, Component, nextProps, updateExpirationTime, renderExpirationTime) {
  if (current$$1 === null) {
    var type = Component.type;

    if (isSimpleFunctionComponent(type) && Component.compare === null && // SimpleMemoComponent codepath doesn't resolve outer props either.
    Component.defaultProps === undefined) {
      var resolvedType = type;

      {
        resolvedType = resolveFunctionForHotReloading(type);
      } // If this is a plain function component without default props,
      // and with only the default shallow comparison, we upgrade it
      // to a SimpleMemoComponent to allow fast path updates.


      workInProgress.tag = SimpleMemoComponent;
      workInProgress.type = resolvedType;

      {
        validateFunctionComponentInDev(workInProgress, type);
      }

      return updateSimpleMemoComponent(current$$1, workInProgress, resolvedType, nextProps, updateExpirationTime, renderExpirationTime);
    }

i will open a issue on React.

So this "issue" is not related to useSelector!

markerikson · 2019-11-08T18:34:42Z

I'm not actually sure what this PR is trying to accomplish. Can you clarify?

Also, whatever it's doing only seems to involve useSelector, and not connect.

salvoravida · 2019-11-08T18:45:44Z

I'm not actually sure what this PR is trying to accomplish. Can you clarify?

Also, whatever it's doing only seems to involve useSelector, and not connect.

https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode
tearing when dispatching fast action and slow updates, please @dai-shi can you add your thought?
fix support dispatch action within "startTransaction"
here demo ok : https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-pytbh
here demo KO : https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-crbfn
isPending is not working

salvoravida · 2019-11-08T18:46:56Z

the problem was reading store on render phase.
The mental modal is -> store Changes -> subcriptions call -> enqueque storeState, NOT forceRender

markerikson · 2019-11-08T18:58:10Z

The point of forceRender is that the Redux store notification process is occurring outside of React, and therefore React has no way to know that a re-render is needed. Therefore, we need to tell React that it should queue up a re-render, and we are literally "forcing a render".

Now, the exact mechanics of that may be up for discussion.

For any potential change, I would specifically like to see some unit tests that fail under the existing implementation, and pass under a new implementation.

I'd also like to know how this affects our performance benchmarks. I'm not sure if we ever actually added the useSelector implementations to our benchmarks repo. @MrWolfZ , are this still only on your box?

salvoravida · 2019-11-08T19:18:37Z

The point of forceRender is that the Redux store notification process is occurring outside of React, and therefore React has no way to know that a re-render is needed. Therefore, we need to tell React that it should queue up a re-render, and we are literally "forcing a render".

Now, the exact mechanics of that may be up for discussion.

For any potential change, I would specifically like to see some unit tests that fail under the existing implementation, and pass under a new implementation.

yes i know. the only change is that reduxStore.getState() must be done ONLY on useEffect and non on render otherwise there is the tearing effect when react render 50 items in a random way.

anyway, ok i will update with 2 test that simulates the 2 demos

markerikson · 2019-11-08T19:20:19Z

Sure. The other half of this is that we need an equivalent implementation for connect(), as well.

dai-shi · 2019-11-08T22:53:21Z

I'm not sure if we ever actually added the useSelector implementations to our benchmarks repo.

@markerikson I did. Please check out the PR and I can adjust if you have suggestions.

dai-shi · 2019-11-08T23:03:39Z

@salvoravida In my experience, if you read store in render, you need to deal with the case that the store state is change from the time when checkForUpdates is called. That's what use-subscription is dealing with.

salvoravida · 2019-11-09T01:11:05Z

Sure. The other half of this is that we need an equivalent implementation for connect(), as well.

done, fixed connect too.
Hope i have some time on weekend to add the CM tests.

Meanwhile if someone would like to try, here is a temp build : @salvoravida/react-redux 7.1.6 🎉

src/components/connectAdvanced.js

MrWolfZ · 2019-11-09T07:16:41Z

test/hooks/useSelector.spec.js

-              if (count !== parentCount) {
-                sawInconsistentState = true
-              }
+              lastRenderInconsistentState = count !== parentCount


I don't understand this change. The whole purpose of this test was to ensure the selector never sees inconsistent state, not just on the latest render.

Because we NO more read the last state in the render phase, the "parent-child-issue" is resolved with 2 render, as every change of the store make 2 event-callback one on the parent and one on the child.
Before the issue was resolved with the "hack" of reading always the fresh Store state, even if we are on the "preview" scheduled refresh came out from "checkForUpdate" callback.

That hack while resolve the "parent-issue" with 1 render less, broke the changes normal queue on the the componente, that produce tearing on CM where every component could rendered in a random way, jumping the "changhes queue"

please try the demo of @dai-shi with my version and with old one. you will see the differences on a fast store changes, with slow render.

MrWolfZ · 2019-11-09T07:18:36Z

test/components/Provider.spec.js

-      expect(childMapStateInvokes).toBe(2)
-      expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']])
+      expect(childMapStateInvokes).toBe(3)
+      expect(childCalls).toEqual([['a', 'a'], ['a', 'ac'], ['ac', 'ac']])


this changes the whole point of the test. this test ensures that mapState never sees inconsistent state. with this change you are asserting the exact opposite

Because we NO more read the last state in the render phase, the "parent-child-issue" is resolved with 2 render, as every change of the store make 2 event-callback one on the parent and one on the child.
Before the issue was resolved with the "hack" of reading always the fresh Store state, even if we are on the "preview" scheduled refresh came out from "checkForUpdate" callback.

That hack while resolve the "parent-issue" with 1 render less, broke the changes normal queue on the the componente, that produce tearing on CM where every component could rendered in a random way, jumping the "changhes queue"

please try the demo of @dai-shi with my version and with old one. you will see the differences on a fast store changes, with slow render.

If you are developing your own fork, it's fine and I think it's one possible approach.
But, if you would like to support all existing RR apps (which is necessary for the official RR), you want to keep the backward compatibility. As I said, it should be technically possible. Seems like you are used to the code base, I think you can do it. I'm not 100% sure though.

@dai-shi i'm not dev my own fork, otherwise i havent opened a pr. that said i think that was a mistake reading fresh store state on render, to have 1 less render on that particular use case. Moreover it is of course back compatible with 1 render more (for that usecase)
Anyway i will try to explain better with a flow chart as soon as i can. That's all.

The Exactly point is: when a componet render it MUST read the storeState that was from HIS last storeChangeCallback!. Otherwise reading in render store.getState() could be newer than HIS callback queque. THAT'S THE POINT. Think more about that. test the demo.

Can you clarify what you mean by "more renders"? It sounds like you're saying we will cause additional render cycles to occur, which is going to be bad for performance.

@markerikson I believe this is not due to an additional render. let me try to outline the order of events here:

the store updates

the parent sees the change first (due to tiered subscription) and renders

since the parent rendered and changed the props, the child renders as well. with this change, during the render the child will see the last state it knows about, which is the previous store state (since due to tiered subscription the child didn't see the update yet). funnily enough this is actually the inverse of the stale props issue, since now we actually see newer parent props with an old state instead of old props with a new state

once the render of the parent finishes the child finally sees the state update and re-renders itself with consistent props and state

it feels to me that tiered subscription and CM might just be incompatible

@salvoravida as @dai-shi pointed out, this discussion is not about the correctness of your change, but about preserving the existing behavior for backwards compatibility. there are likely tons of apps out there that have mapStateToProps written in a way that depends on the props and state being consistent. that's why it is important we think of a way to keep the existing behavior. in the useSelector hook we took the approach of simply ignoring these situations and dealing with errors by forcing a re-render. that was a deliberate decision we could make because it was a new API. connect is sadly different

src/hooks/useSelector.js

salvoravida · 2019-11-10T01:08:14Z

@salvoravida In my experience, if you read store in render, you need to deal with the case that the store state is change from the time when checkForUpdates is called. That's what use-subscription is dealing with.

this pr does not read store on render, but on callback handler.

dai-shi · 2019-11-10T02:11:31Z

this pr does not read store on render, but on callback handler.

Sorry, that's my misunderstanding from your original comment. I will try to understand again how it's working.

Have you come up with any idea to keep the test unchanged?

salvoravida · 2019-11-10T05:21:45Z

Hi folks, i have added dai-shi CM tests on /test/cm !
🎉 Good News ! @dai-shi update your site with react-redux :D

PASS  __tests__/all_spec.js (12.341s)
  react-redux
    √ check1: updated properly (8053ms)
    √ check2: no tearing during update (1ms)
    √ check3: ability to interrupt render
    √ check4: proper update after interrupt (2108ms)

Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total

dai-shi · 2019-11-10T06:13:52Z

Please calm down.
Reading useSelector.js carefully, I think I understand what you are trying to do.
It will be more CM friendly than the current RR, if it could be done right.
To my knowledge, we can't use useRef to keep single source of truth.
I could probably make a failing test, but it would be hard and I wouldn't dare to do it.

So, there are two issues in my mind. One is the changed test case, and the other is the useRef. I'm not sure but they might be from the single issue.

Based on your idea, I would think something like this might work.

const useSelector = (selector) => {
  const [selected, setSelected] = useState(selector(store.getState()));
  if (selected !== selector(store.getState())) {
    setSelected(selector(store.getState()));
  }
  ...
};

Hmm, I'm not sure if this solves the first issue...
(I'm not even sure if the same technique can be applied to connect)

You want to try this? It seems like you are so exited (which is good), and I'm not sure I can convince you.

src/hooks/useSelector.js

salvoravida · 2019-11-11T01:18:34Z

There are so many points that we should discuss and find all an agreement, step by step, without mixing different points.

i will try to start with these:

First of all, we should all agree that we CANNOT read store.getState() into render phase, because that could be "reading from the future" in CM when there are many slow-render-component, dispatches with LOW priority and Scheduler interrupt ROOT_RENDER and commit partial renders.
That is "TEARING" effect.
Second point is that storeChanges MUST be enquequed to useState with setState, so that every render can read reduxState from hisCallback and his queue

this fix also the others @dai-shi CM tests.

This is also why i do NOT think that your last example of useSelector can work in CM!

dai-shi · 2019-11-11T01:41:26Z

I understand we are not on the same page.

[First Goal] My goal is to at least make it work in CM. That is to make my CM tests (excluding check3) pass without changing useSelector.spec.js.
[Future Goal] Your goal is far ahead of it, and try to support state branching. That would be challenging and we might not be able to keep backward compatibility.

we should all agree that we CANNOT read store.getState() into render phase

I disagree if I stand for [First Goal]. For [Future Goal], yes.

salvoravida · 2019-11-11T01:53:37Z

you cannot have the [FG] CM safe (no tearing) while you want to read on render phase!

the test that i have changed is safe, was an hack before to force read store on render phase.
to explain better this, i will prepare new tests. just wait! :)

dai-shi · 2019-11-11T02:04:19Z

you cannot have the [FG] CM safe (no tearing) while you want to read on render phase!

Both are [FG]... I assume [First Goal]. I think we can based on my snippet (which is from use-subscription which is basically for external store for CM safe).

eddyw · 2019-11-11T05:42:00Z

@dai-shi

[Future Goal] Your goal is far ahead of it, and try to support state branching. That would be challenging and we might not be able to keep backward compatibility.

I don't think it's trying to do that. There will always be state branching as long as it uses useState or useReducer, and it'll happen whenever you set a value within a transition. In this case, dispatch an action within a transition. However, that's a completely unrelated issue to "fixing tearing"

If you think in useContextSelector, if you set the value passed to the provider within a transition, it will have this state branching as well.

I'm trying to describe this in the other thread. Also, it'd be backwards compatible of course, because existing RR apps just don't use useTransition and Suspense only supports React.lazy

dai-shi · 2019-11-11T11:32:23Z

@eddyw Thanks for the clarification. I agree fixing tearing is nothing to do with branching.
And I think it can be solved by reading store state in render and fallback to re-render if it's changed during renders. (BTW, your description of the research in the other thread is very interesting.)

salvoravida · 2019-11-11T22:44:01Z

fixed new @dai-shi CM tests. now reduxState is 100% "streamed" from Redux to Components, without reading from the future or jumping queue callbacks! CM should be fine.

dai-shi · 2019-11-11T23:36:40Z

src/hooks/useSelector.js


        if (equalityFn(newSelectedState, latestSelectedState.current)) {
+          setReduxState(prev => {
+            prev.value = newReduxState


@salvoravida As I said in other thread, I'm not confident this works for the future. Do you ever feel like just trying my previous snippet???

For what i have understood, useRef is not CM safe. its a shared mem between branches. we need an useStateRef, a way to save data to current branch states queue.
I think it is safe, just says React to not re-render, but store a value on that branch.

i dont' think that reading value from the future is ok in CM and event streaming, but i will try your idea asap.
Have you tried it?

This line is mutating the existing state value, though, which is definitely not CM-safe.

no, because it is executed only on next update, before next render.

on every branch, so every branch has it's own reduxState queue changes. this is why there is NO more tearing, even if with last test, that simulate this edge case.

hm, so it's more or less like...:

const [refState, forceUpdateRef] = React.useState({ current: null }) React.useImperativeHandle(refState, () => 'some value', [ ...optional_deps ])

Isn't this just like a short cut?, I mean, if keeping state in ref and just using forceUpdate. If it's within a transition, forceUpdate will be scheduled for later but the ref will still change which I think this does. Using useState for refs is just like merging a ref with forceUpdate into one line of code:

const ref = useRef() const forceUpdate = useReducer(c => c + 1, 0)[1] somewhere { ref.current = someValue forceUpdate() } ... const [ref, forceUpdate] = useState({ current: null }) somewhere { ref.current = someValue forceUpdate({ ...ref }) }

Both forceUpdate will schedule "for later" if within transition.

Yes but using ref, another rendering brahch could update the same value before or after, like shared mem between 3d. Instead states ref are duplicated on every branch and every state ref updater will work on its own branch rendering.

Idk, this is how I see it:

queue no transition => [ setReduxState(p => { p.value = reduxState 1; return p }), setReduxState(p => { p.value = reduxState 2; return p }), // before p.value is reduxState 1 setReduxState(p => { p.value = reduxState 3; return p }), // before p.value is reduxState 2 ] start transition queue => [ later setReduxState(p => { p.value = reduxState 4; return p }), // previous 'p' is unknown ] queue outside transition => [ setReduxState(p => { p.value = reduxState 5; return p }), // before p.value is reduxState 3 ] stop transition queue => [ run previous: initial state: reduxState 5 <<<< this is previous 'p' now setReduxState(p => { p.value = reduxState 4; return p }), // <<< inconsistent state ]

Now, current @dai-shi tests using transition are not exactly right. It's testing dispatching in transition.. but, since there is no Suspense, no component is waiting for anything, so there is no need to queue for later.

Sometimes there may be some flickering on screen, when you use transition but nothing causes interrupting of rendering (like throw promise on Suspense), I've seen on some demos I was testing with, I think this is a bug related to what Dan mentioned on twitter, maybe? Point is, you can't test using transition without causing interrupt on render (throw on Suspense), otherwise it is pointless to wrap it in startTransition.

I'm not following the discussion in deep, but:

without causing interrupt on render (throw on Suspense)

My understanding is a bit different. We can cause interrupt with heavy computation without throwing promise. That's the whole point of my tests.

P.S. Did I ever test using transition? Just today, I made new checks using useTransition based on @eddyw 's demo. No Suspense. No throwing promises. Is it pointless?

Never mind, ignore the above 🙈
I've copied the tests over this PR locally which did have useTransition but you finished committing later, so I had them broken

markerikson · 2021-04-07T02:00:36Z

This PR is stale, and it's unlikely we're going to make any CM-specific changes to v7 at this point. Instead, we're more likely to rewrite things into a v8 release that builds on top of useMutableSource somehow.

fix cm use selector

b986030

salvoravida mentioned this pull request Nov 8, 2019

Concurrent Mode #1351

Closed

salvoravida mentioned this pull request Nov 8, 2019

Feedback from salvoravida dai-shi/will-this-react-global-state-work-in-concurrent-rendering#3

Closed

eddyw reviewed Nov 8, 2019

View reviewed changes

salvoravida added 3 commits November 9, 2019 01:28

fix cm on connect

9c0520f

update test for child-parent double render

3d52f3a

save a ref when selector does not trigger an update

647195d

markerikson requested changes Nov 9, 2019

View reviewed changes

src/components/connectAdvanced.js Outdated Show resolved Hide resolved

MrWolfZ reviewed Nov 9, 2019

View reviewed changes

src/hooks/useSelector.js Outdated Show resolved Hide resolved

add CM tests

d248d0a

eddyw reviewed Nov 10, 2019

View reviewed changes

src/hooks/useSelector.js Outdated Show resolved Hide resolved

fix possible falsy on reduxState

eb6f8cf

salvoravida added 3 commits November 11, 2019 16:21

update cm tests @dai-shi

6647836

removed useRef - fixed new test on CM

5ed652c

removed useRef - fixed new test on CM

16a6253

dai-shi reviewed Nov 11, 2019

View reviewed changes

dai-shi mentioned this pull request Jan 27, 2020

A possible idea towards fixing useSelector tearing in Concurrent Mode #1509

Closed

markerikson closed this Apr 7, 2021

		@@ -31,7 +31,7 @@ function useSelectorWithStoreAndSubscription(
		selector !== latestSelector.current \|\|
		latestSubscriptionCallbackError.current

fix CM tearing #1455

fix CM tearing #1455

Conversation

salvoravida commented Nov 8, 2019

netlify bot commented Nov 8, 2019 • edited Loading

salvoravida commented Nov 8, 2019

dai-shi commented Nov 8, 2019

salvoravida commented Nov 8, 2019

salvoravida commented Nov 8, 2019

eddyw commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvoravida commented Nov 8, 2019 • edited Loading

markerikson commented Nov 8, 2019

salvoravida commented Nov 8, 2019

salvoravida commented Nov 8, 2019 • edited Loading

markerikson commented Nov 8, 2019 • edited Loading

salvoravida commented Nov 8, 2019

markerikson commented Nov 8, 2019

dai-shi commented Nov 8, 2019 • edited Loading

dai-shi commented Nov 8, 2019

salvoravida commented Nov 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvoravida Nov 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvoravida commented Nov 10, 2019

dai-shi commented Nov 10, 2019

salvoravida commented Nov 10, 2019 • edited Loading

dai-shi commented Nov 10, 2019

salvoravida commented Nov 11, 2019 • edited by timdorr Loading

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019 • edited by timdorr Loading

dai-shi commented Nov 11, 2019

eddyw commented Nov 11, 2019 • edited Loading

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

Choose a reason for hiding this comment

salvoravida Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvoravida Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyw Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Apr 7, 2021

netlify bot commented Nov 8, 2019 •

edited

Loading

salvoravida commented Nov 8, 2019 •

edited

Loading

salvoravida commented Nov 8, 2019 •

edited

Loading

markerikson commented Nov 8, 2019 •

edited

Loading

dai-shi commented Nov 8, 2019 •

edited

Loading

salvoravida Nov 9, 2019 •

edited

Loading

salvoravida commented Nov 10, 2019 •

edited

Loading

salvoravida commented Nov 11, 2019 •

edited by timdorr

Loading

salvoravida commented Nov 11, 2019 •

edited by timdorr

Loading

eddyw commented Nov 11, 2019 •

edited

Loading

salvoravida Nov 12, 2019 •

edited

Loading

salvoravida Nov 12, 2019 •

edited

Loading

eddyw Nov 12, 2019 •

edited

Loading