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

Component not aware of state change? #20

Closed
mrGibi opened this issue Apr 5, 2019 · 29 comments
Closed

Component not aware of state change? #20

mrGibi opened this issue Apr 5, 2019 · 29 comments

Comments

@mrGibi
Copy link

mrGibi commented Apr 5, 2019

I've been fighting this all day today.
Im new to Java/TypeScript world, React world and Redux world and I was pretty sure that I messed something up... until I tried doing the same with React-Redux (7.0.0.beta-1) and it turned out it works there!

Im writing a authorization flow with redux-saga - so this example that Im going to show you is just massively simplified code similar to mine. CodeSandboxes below.

Description: TOGGLE button dispatches an action that is caught by redux-saga. The Saga dispatches an action to toggle a value to true, wait 10ms, and then dispatches another action to toggle value back to false. Meanwhile the button is disabled if value === true, and there is a value ? 'ON' : 'OFF' text. Interestingly if you increase delay() value to (for example) 50ms it works!

How does it look like:

  • using react-redux 7.0.0-beta.1 (good!)
    react-redux

  • using reactive-react-redux 1.8.0 (bad!)
    reactive-react-redux

This was shot on MacOs 10.14.4 and Safari 12.1.
I have tested this also on:

  • iOS: latest Safari, latest Chrome and latest Firefox. Results are the same, although on Firefox sometimes I had to click "toggle button" couple of times to "achieve" this issue.
  • MacOs: latest Chrome, latest Firefox. On Firefox and Chrome sometimes it needs couple of clicks to get there.

Check out these codesandboxes:

I dropped on this issue by accident. In my original code the delay function is just a Api call. This Api call had a wrong port number set so it got Connection refused and returned immediately. TURN_ON and TURN_OFF was just a isLoading indicator that disabled a Login button.

@dai-shi Im totally blown away by Your work on reactive-react-redux and @theKashey 's proxyequal. I follow every discussion, every benchmark test and commits to this repo and wish I would understand enough of it to be able to help...

@theKashey
Copy link

theKashey commented Apr 5, 2019

Just give an example to fight with! (ok, there is a sandboxes)

@theKashey
Copy link

Working on Chrome, not working on Safari or FF

@theKashey
Copy link

In short - this is due to batchedUpdates. But it's not clear why it does not work as expected.

@mrGibi
Copy link
Author

mrGibi commented Apr 5, 2019

@theKashey latest Chrome? On mine its not working on at most second/third click.
Another example, much simpler, based on @dai-shi codesandboxes from README.md: my codesandbox fork. There's a "multiple" button which triggers two dispatch(), one after the other. Component is not updating until you click "+1" or "-1" buttons.

@dai-shi
Copy link
Owner

dai-shi commented Apr 5, 2019

Thanks for the example. You are already helping very much with it. @mrGibi

I think this might be related with my thoughts on #15.
Could you try this commit 10c9574 in store-state-for-updates branch?
(I'm trying it too, but codesandbox doesn't accept installing a npm package with commit???)

@theKashey
Copy link

So - by the time second update hits comparison - the last state is not yet stored in lastProxyfied.current.state

  • store update
  • force update
  • component rerender with a new store
  • use effect to save a new store
  • store update
  • comparison with an old store
  • no update
  • lastProxyfied.current.state update

@theKashey
Copy link

@dai-shi - useProxyfied should use useLayoutEffect

@theKashey
Copy link

In short - a new store update got triggered while an old update is not fully "commited".

  • or use layoutEffect - it's almost sync after the render
  • or update lastProxyfied.current.state after successful comparison (you have "used" it)

@dai-shi
Copy link
Owner

dai-shi commented Apr 5, 2019

useProxyfied should use useLayoutEffect

Oh... I thought about that before, but yeah.
Does this #20 (comment) also help?? (maybe not. it's a different problem.)

(I've got to leave now... 🚗 )

@theKashey
Copy link

Yeah, concurrent mode. Actually, we have got tearing here, when different pieces are using states from different times.
So

  • use layoutEffect to close rendering, or else you may record proxy results from more than one render.
  • update lastProxyfied.current.state in the comparison function just after comparison, cos you dont need to defer it to record used keys, and you have to update it, as long as comparison with a "real" state was just made, and you dont want to redo it again, which would be the case, if state got changed one more time before a real render take a place.

@mrGibi
Copy link
Author

mrGibi commented Apr 6, 2019

Could you try this commit 10c9574 in store-state-for-updates branch?

Unfortunately this doesnt resolve the issue 😔

@dai-shi
Copy link
Owner

dai-shi commented Apr 6, 2019

Good to know. I will make a useLayoutEffect version. I’m not sure if I understand the second bullet, yet.

@theKashey
Copy link

  • state1 {a:1, b:1}, we are using only a.
  • component render
  • inner state update
  • state2 update {a:2, b:2} - change detected, force update
  • state3 update {a:2, b:3} - change detected, force update

Internal state was not yet updated, as a result the second comparison with an old state and a quick state update is a false positive, and might took more time than a comparison with state2.

So - once you compare a and b - set a=b

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

Got it. It’s false positives. I wonder if that would cause other false negatives in some edge cases?

@theKashey
Copy link

Should not. You already triggered redraw, and it would commit the "used" state in a proper time.
So - even if by any reason (Concurrent) render did happen with older state - useLayoutEffect would update the record as it should - "state used by fact".

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

OK, let me make a fix.

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

Here you go: db485e0
Would you try with this commit? @mrGibi

@mrGibi
Copy link
Author

mrGibi commented Apr 7, 2019

db485e0 didnt help 😔

Since you pushed to master, we can use it on Codesandbox by using:

"dependencies": {
    // ...
    "reactive-react-redux": "github:dai-shi/reactive-react-redux",
    // ...
  },

in package.json. Click here to open this codesandbox.

Its based on your example from README.md but with additional multiple button that triggers two dispatch() in Counter.tsx. As you can see, clicking multiple button doesnt trigger component update.

Additionaly you can use my example repo on localhost with this:

git clone https://github.com/mrGibi/reactive-react-redux-tests.git
cd reactive-react-redux-tests/
npm install
npm start

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

Oh... It wasn't about useLayoutEffect nor updating state.
It's about useForceUpdate. @theKashey
I didn't expect useReducer bails out for boolean.

Interestingly, if you add one more dispatch, it works. @mrGibi

const hc = () => {
  dispatch({ type: "increment" });
  dispatch({ type: "increment" });
  dispatch({ type: "increment" });
};

If you have another to make them four, it won't.

@mrGibi
Copy link
Author

mrGibi commented Apr 7, 2019

I dont get it...

I had two examples showing the issue:

yield put({ type: "TURN_ON" });
yield delay(10);
yield put({ type: "TURN_OFF" });
const hc = () => {
  dispatch({ type: "increment" });
  dispatch({ type: "increment" });
};

I thought that both examples are pretty much identical, but the second one was just simplified version of the first one. Its all about making two (or more) fast changes to the state and component not being aware of those changes, right?

Well it turns out that they are not as similar as I thought, becouse after your latest commit to master, the first example (now using latest reactive-react-redux from github/master) is working correctly. See here.

And the second example with:

const hc = () => {
  dispatch({ type: "increment" });
  dispatch({ type: "increment" });
};

now using latest reactive-react-redux from github/master (codesandbox) still doesnt work 🤷‍♂️

dai-shi added a commit that referenced this issue Apr 7, 2019
@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

Please try the latest master.

@mrGibi
Copy link
Author

mrGibi commented Apr 7, 2019

Ok, both examples are working now. Thanks!

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

So, the question remained to me is whether we really need useLayoutEffect. @theKashey

  • state1 {a:1, b:1}, we are using only a.
  • component render
  • committed & inner state update
  • state2 update {a:2, b:2} - change detected, force update
  • component render, which uses b
  • state3 update {a:2, b:3} - no change detected
  • committed & inner state update
  • no update (false negatives)

So, your approach totally depends on useLayoutEffect run 100% sync.

@theKashey
Copy link

Yes. As long as you might have multiple renders before single useEffect, and you have to track purely only one render - you have to useLayoutEffect. You have to "close" tracking just after render.

@dai-shi
Copy link
Owner

dai-shi commented Apr 7, 2019

If we both revert useLayoutEffect and "close" tracking", it still works (no false negatives).
We have false positives (#20 (comment)), though it's different from what is happening to @mrGibi 's example.

If both are correct, oh and if we solve the concurrent mode issue in the future, we don't need useLayoutEffect.
I just feel like useLayoutEffect is a last resort we'd like to avoid unless we have a real problem.

@dai-shi
Copy link
Owner

dai-shi commented Apr 9, 2019

@theKashey I also want your comment on this.

@theKashey
Copy link

If we both revert useLayoutEffect and "close" tracking", it still works (no false negatives).

🤷‍♂️ I am not sure why it works. Give me some time to play with it.

@dai-shi
Copy link
Owner

dai-shi commented Apr 12, 2019

If both are correct, oh and if we solve the concurrent mode issue in the future, we don't need useLayoutEffect.

This "if we solve the concurrent mode issue" is kind of strong condition more than I thought on the previous day.
I saw useIsomorphicLayoutEffect in react-redux thread, which works around the warning issue, so I'd follow that.

@dai-shi
Copy link
Owner

dai-shi commented Apr 13, 2019

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

No branches or pull requests

3 participants