Feedback from salvoravida

[dai-shi]

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]

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]

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]

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]

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

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

[salvoravida]

FYI reduxjs/react-redux#1455

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

[salvoravida]

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

[dai-shi]

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]

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

[dai-shi]

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

[salvoravida]

🎉 "@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]

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]

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]

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]

facebook/react#17318

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

[dai-shi]

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