Feedback from salvoravida #3

dai-shi · 2019-11-08T04:39:30Z

reduxjs/react-redux#1351 (comment)
@salvoravida gave some feedbacks. As far as I know you are the forth person who read this project carefully. Just an estimate.

dai-shi · 2019-11-08T04:47:43Z

an externa button is not "remote" low priorty work

Originally, I wanted to emulate actions from the network.
What would be the priority if we issue an action from the network event?

"reactive-react-redux" is not so "reactive"

Well, the reactive part I took is from "useTrackedState".

you will see jumps in the counter on screen when click fast.

That's what I expect. React is throwing away intermediate screens.
I wanted to replicate the behavior of the pure context lib like constate.

tearing is not a bug

I wouldn't disagree here. It's a behavior. I think most of apps do work without noticing it.
It's a hypothetical issue that some apps might get affected.

dai-shi · 2019-11-08T04:59:34Z

It's a hypothetical issue

But, according to Talk by Flarnie Marchan, it was a real problem in Facebook.

The question is if this project is replicating the same issue.

salvoravida · 2019-11-08T05:23:47Z

t is replicating the same issue.

i have not seen all the video, but it seems that it comes out with incorrect state rendered, not tearing.
with react-redux at the end you will get correct rendering.

Am i missing something?

dai-shi · 2019-11-08T06:23:32Z

with react-redux at the end you will get correct rendering.

Yes, that is what I check by "check1".

salvoravida · 2019-11-08T12:36:30Z

FYI reduxjs/react-redux#1455

after some more investigation i see that reading store.state on render phase was the probelm,
not priority

salvoravida · 2019-11-08T12:51:36Z

can you try with this temp build "@salvoravida/react-redux": "^7.1.4",

dai-shi · 2019-11-08T13:27:06Z

I think the approach is good. It has to take care about store state updated during react rendering. That's technically possible either with useLayoutEffect which run in sync or something like use-subscription. This issue is not covered by my checks in this project, because it's more important than tearing and assumed to be solved.
I'm interested in trying your fork, especially if you fix the issue and you should be able to do it.

salvoravida · 2019-11-08T13:35:52Z

sorry i cannot understand, what issue are you talking about?

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

Let me take some time to come up with an idea to check it, so that I can explain better with examples.

salvoravida · 2019-11-10T05:25:38Z

🎉 "@salvoravida/react-redux": "^7.1.6"

 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

salvoravida · 2019-11-10T05:26:56Z

Note: you should update your demo

const _Counter = () => {
  const count = useSelector(state => state.count);
  syncBlock();
  return <div className="count">{count}</div>;
};

_Counter.defaultProps={};
//https://github.com/facebook/react/issues/17318  
//currently 2019.11.10 SimpleMemoCompnent is buggy on CM

const Counter=React.memo(_Counter);

dai-shi · 2019-11-10T05:29:09Z

Wow, looks like you know how all things work.
Would you take a look at use-subscription case why it's failing with check4, when you have time? I'm so curious about it, but I don't understand what is happening.

salvoravida · 2019-11-10T05:46:23Z

Wow, looks like you know how all things work.
Would you take a look at use-subscription case why it's failing with check4, when you have time? I'm so curious about it, but I don't understand what is happening.

const _Counter = () => {
  const count = useSubscription(React.useMemo(() => ({
    getCurrentValue: () => store.getState().count,
    subscribe: (callback) => {
      return store.subscribe(callback);
    },
  }), []));
  syncBlock();
  return <div className="count">{count}</div>;
};
_Counter.defaultProps = {};
const Counter = React.memo(_Counter);

now it's ok!

  console.log __tests__/all_spec.js:43
    use-subscription [
      191, 161, 161, 181,
      181, 183, 181, 180,
      192, 190
    ]
 PASS  __tests__/all_spec.js (13.017s)
  use-subscription
    √ check1: updated properly (8558ms)
    √ check2: no tearing during update (2ms)
    √ check3: ability to interrupt render
    √ check4: proper update after interrupt (1030ms)

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

salvoravida · 2019-11-10T05:47:12Z

facebook/react#17318

useSubscription is OK! React.memo in SimpleMemo version NO! 👯‍♂

dai-shi · 2019-11-10T06:18:19Z

So so so amazing!!!
Looks like you did a great job!!!
I'll take a deeper look at the issue. Thanks so much!

dai-shi · 2019-11-10T07:06:58Z

I confirmed this patch makes use-subscription's check4 to pass.

diff --git a/src/use-subscription/index.js b/src/use-subscription/index.js
index 3bc6f4e..180ab62 100644
--- a/src/use-subscription/index.js
+++ b/src/use-subscription/index.js
@@ -21,7 +21,7 @@ const Counter = React.memo(() => {
   }), []));
   syncBlock();
   return <div className="count">{count}</div>;
-});
+}, () => true);
 
 const Main = () => {
   const count = useSubscription(React.useMemo(() => ({

dai-shi · 2019-11-10T09:40:09Z

@samcooke98 You might want to try this workaround until it's resolved. (If you have props, use shallowEqual).

samcooke98 · 2019-11-10T23:34:30Z

Thanks for the tag.

I've added a failing test to React here: facebook/react#17336

dai-shi · 2019-11-11T05:35:57Z

fb5c61a

@salvoravida I added your fork and failing test case.

  salvoravida-react-redux
    ✓ check1: updated properly (8992ms)
    ✕ check2: no tearing during update (1ms)
    ✓ check3: ability to interrupt render (1ms)
    ✓ check4: proper update after interrupt (1259ms)

salvoravida · 2019-11-11T08:12:02Z

fb5c61a

@salvoravida I added your fork and failing test case.

  salvoravida-react-redux
    ✓ check1: updated properly (8992ms)
    ✕ check2: no tearing during update (1ms)
    ✓ check3: ability to interrupt render (1ms)
    ✓ check4: proper update after interrupt (1259ms)

Have you change the test?

dai-shi · 2019-11-11T08:20:18Z

Yes, look at the commit. fb5c61a

salvoravida · 2019-11-11T09:10:33Z

Yes, look at the commit. fb5c61a

yes i understand, i was already working on it. the problem is useRef.

salvoravida · 2019-11-11T11:57:28Z

PASS tests/all_spec.js (12.425s)
salvoravida-react-redux
√ check1: updated properly (8061ms)
√ check2: no tearing during update (2ms)
√ check3: ability to interrupt render
√ check4: proper update after interrupt (2293ms)

"@salvoravida/react-redux": "^7.1.8",

dai-shi · 2019-11-11T11:59:41Z

That was quick! You eliminated the useRef?

salvoravida · 2019-11-11T12:02:51Z

That was quick! You eliminated the useRef?

yes useRef is not enquequed so is not CM safe.

on SelectedState NOT changed

          setReduxState(prev => {
            prev.value = newReduxState
            return prev
          })

on change
setReduxState(() => ({ value: newReduxState }))

in this way, reduxState is correctly enququed and React will discard updates with prevState=nextState.

salvoravida · 2019-11-11T12:05:05Z

the only "problem" is this test:

   it('uses the latest selector', () => {
        let selectorId = 0
        let forceRender

        const Comp = () => {
          const [, f] = useReducer(c => c + 1, 0)
          forceRender = f
          const renderedSelectorId = selectorId++
          const value = useSelector(() => renderedSelectorId)
          renderedItems.push(value)
          return <div />
        }

        rtl.render(
          <ProviderMock store={store}>
            <Comp />
          </ProviderMock>
        )

        expect(renderedItems).toEqual([0])

        rtl.act(forceRender)
        expect(renderedItems).toEqual([0, 1])

        rtl.act(() => {
          store.dispatch({ type: '' })
        })
        expect(renderedItems).toEqual([0, 1])

        rtl.act(forceRender)
        expect(renderedItems).toEqual([0, 1, 2])
      })

because React will render and discard the render.

But i really think that this test is NOT ok. is it not safe upadate var on render,
due to the fact that a Render should be Pure, as it is not garatee that will be committed!

dai-shi · 2019-11-11T12:06:20Z

I see tearing when I press "Increment local count" many times during updating.

salvoravida · 2019-11-11T12:17:36Z

hum i cannot replicate what you say, to me it seems ok

are you sure have update to 7.1.8 ?

dai-shi · 2019-11-11T12:22:38Z

I try several times, but it's not easy to reproduce this error. It only happens rarely. Yeah, but it's reproducible.

salvoravida · 2019-11-11T12:23:40Z

I try several times, but it's not easy to reproduce this error. It only happens rarely. Yeah, but it's reproducible.

can you make a test?
it will happen also on others lib?

dai-shi · 2019-11-11T12:23:50Z

$ md5sum dist/salvoravida-react-redux/main.js
d384f69057acf852785e18f856864a82  dist/salvoravida-react-redux/main.js
$ jq .version node_modules/@salvoravida/react-redux/package.json
"7.1.8"

dai-shi · 2019-11-11T12:24:55Z

I don't think I can make an automated test for this. Let me try with other libs.

dai-shi · 2019-11-11T12:27:36Z

Okay, I was able to reproduce it with use-subscription. So, it might be React issue.

Let's ignore this unless we find a better way to reproduce it.

dai-shi · 2019-11-11T13:13:46Z

prev.value = newReduxState

This is a nice hack, but it's uncertain if this works in the future, even if it's working now.
For example, React may freeze the state object in development mode.

BTW, as you are so powerful in this field, I would like have your opinion on my lib.
https://github.com/dai-shi/reactive-react-redux
It solve the tearing issue with the approach similar to react-redux v6, still it's performant like react-redux v7 because it's also using subscriptions.

salvoravida · 2019-11-11T13:27:50Z

hum state is propagate 2 times, one via listeners and one via Context, how does impact on performances?
have you tests?

salvoravida · 2019-11-11T13:34:01Z

what do you think abot 'uses the latest selector'
?

dai-shi · 2019-11-11T13:36:50Z

have you tests?

Yep.

https://github.com/dai-shi/reactive-react-redux#benchmarks
Compare reactive-react-redux-useSelector-v4.1.0 and react-redux-hooks-v7.1.0.

reduxjs/react-redux-benchmarks#26
Compare useSelector-rrr-4.2.0 and useSelector-7.1.0.

salvoravida · 2019-11-11T13:46:18Z

have you tests?

Yep.

https://github.com/dai-shi/reactive-react-redux#benchmarks
Compare reactive-react-redux-useSelector-v4.1.0 and react-redux-hooks-v7.1.0.

reduxjs/react-redux-benchmarks#26
Compare useSelector-rrr-4.2.0 and useSelector-7.1.0.

great! why it is not rendered 2 times? one from listener callback and one from context change?O_O

dai-shi · 2019-11-11T13:49:31Z

great! why it is not rendered 2 times? one from listener callback and one from context change?O_O

Because it's using changedBits=0 hack. Please read this summary: dai-shi/reactive-react-redux#29

dai-shi · 2019-11-11T13:52:52Z

what do you think abot 'uses the latest selector'

It looks ok to me. Whether it's mutating in render is not important. It's just testing to read the latest selector (maybe from props. In that sense, there can be a better test, but what it tests would be the same.)
I'm curious, why is this test problematic in your implementation?

salvoravida · 2019-11-11T14:04:26Z

what do you think abot 'uses the latest selector'

It looks ok to me. Whether it's mutating in render is not important. It's just testing to read the latest selector (maybe from props. In that sense, there can be a better test, but what it tests would be the same.)
I'm curious, why is this test problematic in your implementation?

i think that this test is not safe in CM. renders may be discarded. prevState=>prevStates seems to re-render but than scarted due to prevState=newState.

useSubscription use this pattern. i think that this test will fails witn useSubscription too.

salvoravida · 2019-11-11T14:06:37Z

Because it's using changedBits=0 hack. Please read this summary: dai-shi/reactive-react-redux#29

hum and what about zombie child bug? with the hack you loos the benefit of top down context propagation. in this way it seems totally useless, is it just a shared mem ?

am i missing something?

dai-shi · 2019-11-11T14:28:10Z

hum and what about zombie child bug?

It's the same as the stale props issue, right? Either my approach or the current react-redux v7 solves the stale props issue.

(BTW the stale props issue is only applicable to useSelector whereas useTrackedState doesn't have the issue anyway.)

you loos the benefit of top down context propagation.

Yeah, my approach doesn't get the benefit of top-down context propagation much (not zero). It's basically the same as batchedUpdates in react-redux.

am i missing something?

No, I think you are on the right track.

So, the difference between reactive-react-redux and react-redux is subtle. But, this little difference solves the CM tearing issue, because we read from a single react state (not redux store state) through context.

is it just a shared mem ?

So, yes. It's just a shared mem, but this shared mem is managed by React. That's the point.

salvoravida · 2019-11-12T15:17:34Z

@dai-shi new tests? yes i do not have yet time to look at, but i think we need streaming states that sometimes selector get and sometime no, while parent what to re-render for Others props, that modifie selector. This should be the worst case.

have you tested @salvoravida/react-redux 7.1.9

dai-shi · 2019-11-12T15:24:54Z

Yeah, just finished. It was hard to make automated tests, whereas it was easy doing it by hand.
This is basically testing "state branching" as eddyw revealed. My two libs are failing because of external stores. can't be helped.

we need streaming states

I'm not sure if I understand this. Could you elaborate it with examples when you have time?

salvoravida · 2019-11-12T15:36:17Z

have you tested @salvoravida/react-redux 7.1.9 ?

salvoravida · 2019-11-12T16:01:07Z

salvoravida-react-redux 7.1.9
check with events from outside
√ check 1: updated properly (8656ms)
√ check 2: no tearing during update (1ms)
√ check 3: ability to interrupt render
√ check 4: proper update after interrupt (2112ms)
check with useTransaction
√ check 5: updated properly with transition (3387ms)
√ check 6: no tearing with transition (2ms)
× check 7: proper branching with transition (5406ms)

that's great. as all of us expected branching in redux is not supported currently.

but i'm thinking another test. we should have a test that for every reduxState Changes,
sometimes selector schedule an update ad sometimes no.
Meanwhile another event changer from internal ReactTree should change on interval,
ChildProps for schedule a rendering that also invalidate the selector that should depents on props.

This is the worst case.

salvoravida · 2019-11-12T16:58:43Z

that's another point that should be better tested, localIncrement is fired from React internal button.onClick that is wrapped under Scheduler.runWithUserBlockingPpriority, that will force React to not do concurrent work. So localIncrement should be fired in a different way.

dai-shi · 2019-11-12T23:40:23Z

Hm, I'm not following your suggestion. Are there two points? Please help me understand them...

I know localIncrement is in user-blocking priority. That was OK for the "check 4". Are you saying lowering the priority may cause an issue?

sometimes selector get and sometime no

Isn't it sufficient with the current count and dummy?

MrWolfZ · 2019-11-23T09:41:16Z

Yeah, just finished. It was hard to make automated tests, whereas it was easy doing it by hand.
This is basically testing "state branching" as eddyw revealed. My two libs are failing because of external stores. can't be helped.

@dai-shi I am not sure I understand the state branching issue. Can you point me to where eddyw explains it?

dai-shi · 2019-11-23T10:02:56Z

@MrWolfZ In the thread starting from this comment: reduxjs/react-redux#1351 (comment)

dai-shi · 2020-01-17T13:57:56Z

Closing this. Please open a new issue for further discussion.

salvoravida mentioned this issue Nov 12, 2019

fix CM tearing reduxjs/react-redux#1455

Closed

dai-shi closed this as completed Jan 17, 2020

Feedback from salvoravida #3

Feedback from salvoravida #3

Comments

dai-shi commented Nov 8, 2019

dai-shi commented Nov 8, 2019

dai-shi commented Nov 8, 2019

salvoravida commented Nov 8, 2019 • edited Loading

dai-shi commented Nov 8, 2019

salvoravida commented Nov 8, 2019

salvoravida commented Nov 8, 2019

dai-shi commented Nov 8, 2019

salvoravida commented Nov 8, 2019

dai-shi commented Nov 8, 2019

salvoravida commented Nov 10, 2019

salvoravida commented Nov 10, 2019 • edited Loading

dai-shi commented Nov 10, 2019

salvoravida commented Nov 10, 2019

salvoravida commented Nov 10, 2019 • edited Loading

dai-shi commented Nov 10, 2019

dai-shi commented Nov 10, 2019

dai-shi commented Nov 10, 2019

samcooke98 commented Nov 10, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019 • edited Loading

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019 • edited Loading

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019 • edited Loading

dai-shi commented Nov 11, 2019

dai-shi commented Nov 11, 2019 • edited Loading

dai-shi commented Nov 11, 2019 • edited Loading

salvoravida commented Nov 11, 2019

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

dai-shi commented Nov 11, 2019

dai-shi commented Nov 11, 2019

salvoravida commented Nov 11, 2019

salvoravida commented Nov 11, 2019 • edited Loading

dai-shi commented Nov 11, 2019

salvoravida commented Nov 12, 2019

dai-shi commented Nov 12, 2019

salvoravida commented Nov 12, 2019

salvoravida commented Nov 12, 2019

salvoravida commented Nov 12, 2019 • edited Loading

dai-shi commented Nov 12, 2019

MrWolfZ commented Nov 23, 2019

dai-shi commented Nov 23, 2019

dai-shi commented Jan 17, 2020

salvoravida commented Nov 8, 2019 •

edited

Loading

salvoravida commented Nov 10, 2019 •

edited

Loading

salvoravida commented Nov 10, 2019 •

edited

Loading

salvoravida commented Nov 11, 2019 •

edited

Loading

salvoravida commented Nov 11, 2019 •

edited

Loading

dai-shi commented Nov 11, 2019 •

edited

Loading

dai-shi commented Nov 11, 2019 •

edited

Loading

dai-shi commented Nov 11, 2019 •

edited

Loading

salvoravida commented Nov 11, 2019 •

edited

Loading

salvoravida commented Nov 12, 2019 •

edited

Loading