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

useReducer: dispatched actions are reduced twice #14360

Closed
ioss opened this issue Nov 30, 2018 · 4 comments
Closed

useReducer: dispatched actions are reduced twice #14360

ioss opened this issue Nov 30, 2018 · 4 comments

Comments

@ioss
Copy link
Contributor

ioss commented Nov 30, 2018

Do you want to request a feature or report a bug?
Report a bug

What is the current behavior?
There are cases where actions dispatched by useReducer's dispatch function are reduced twice (in concurrent mode).

As far as I can tell from my first quick read of the code of react-dom (so I might be totally wrong!), this happens, when a higher priority action is added to a queue that already contains lower priority actions.

On the first run, that is caused by for example an onClick handler (higher priority), the dispatched action is appended to the queue. On processing the queue the lower priority actions (for example from setInterval events) are ignored and the "onClick action" is reduced, but stays on the end of the queue, so that on the next time the queue is processed, that action gets reduced a second time.

See a testcase at:
https://codesandbox.io/s/nxq3w7rwl

Pressing the button increments the button (most of the time not always, depending if 'TICK' actions are in the queue) by two.

What is the expected behavior?
I'd expect a dispatched action to be only reduced once.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Tested with React 16.7.0-alpha.2 on Chrome (70.0.3538.110) and Safari (12.0.1) both on macOs

@gaearon
Copy link
Collaborator

gaearon commented Nov 30, 2018

This does look like a bug. Want to dig into it?

@ioss
Copy link
Contributor Author

ioss commented Nov 30, 2018

I'd love to, but I am not sure I understand the implications on messing with the action queue, there are quit a lot of special cases and as I wrote that was my first read of any react implementation code :)

Also it might take me a week or so, as I am pretty busy at the moment.

@ioss
Copy link
Contributor Author

ioss commented Nov 30, 2018

Actually I was wrong, the higher priority action should be reduced twice, the error is in cloneHook where the memoizedState (action applied) overwrites the baseState (which should stay the last state before updates were skipped).

I have a test that reproduced my bug and is fixed by correcting cloneHook (while the other tests are still passing). I'll have that cleaned up tomorrow or Monday, so I'd take this issue.

ioss added a commit to ioss/react that referenced this issue Nov 30, 2018
It was broken because `cloneHook` assigned `memoizedState` instead of
`baseState` from the original hook to `baseState` of the clone.
@gaearon
Copy link
Collaborator

gaearon commented Nov 30, 2018

Nice find :-)

gaearon pushed a commit that referenced this issue Nov 30, 2018
* Fixes #14360 and adds a test for mixed priority dispatches.

It was broken because `cloneHook` assigned `memoizedState` instead of
`baseState` from the original hook to `baseState` of the clone.

* tweak comments
jetoneza pushed a commit to jetoneza/react that referenced this issue Jan 23, 2019
* Fixes facebook#14360 and adds a test for mixed priority dispatches.

It was broken because `cloneHook` assigned `memoizedState` instead of
`baseState` from the original hook to `baseState` of the clone.

* tweak comments
n8schloss pushed a commit to n8schloss/react that referenced this issue Jan 31, 2019
* Fixes facebook#14360 and adds a test for mixed priority dispatches.

It was broken because `cloneHook` assigned `memoizedState` instead of
`baseState` from the original hook to `baseState` of the clone.

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

No branches or pull requests

2 participants