reazen/relude-reason-react

Reducer called twice when using ReludeReact.Reducer.useReducer

Closed this issue · 9 comments

It seems that the reducer function is called twice, only the first time, when an action is dispatched:

let (count, dispatch) =
    ReludeReact.Reducer.(
      useReducer(
        count =>
          fun
          | `increment => {
              Js.log("Increment");
              Update(count + 1);
            }
          | `decrement => {
              Js.log("Decrement");
              Update(count - 1);
            },
        0,
      )
    );

// Later...

<div>
  <div> {c->string_of_int->React.string} </div>
  <button onClick={_ => d(`increment)}> "Increment"->React.string </button>
  <button onClick={_ => d(`decrement)}> "Decrement"->React.string </button>
</div>

In the above example, by clicking on the increment button for instance, the console will display the message "Increment" twice on the first click event. The update itself is performed only once. Same goes when using IO, the io code will be called once.

Since calls to log should be wrapped in an IO, the above issue is not critical, but it can be misleading when debugging the code, and it's probably not the expected behavior.

Thanks for the issue report - I'll take a look.

Well, I reproduced the issue, but I'm not quite sure what's going on. There appears to be some unexpected interaction with React useReducer or useEffect1 that's causing it to invoke the reducer twice the first time, which causes that Js.log to happen twice. I tried making some small changes, commenting things out, etc. but wasn't able to find the cause, so I'm not sure what's going on at this point, but we'll keep looking.

That said, you don't have to use an IO for logging or other side effects - you can do this UpdateWithSideEffect(count + 1, _ => Js.log("Increment")), but that doesn't fix the issue at hand, just works around it.

I tried doing the same thing with just a vanilla React.useReducer and the issue doesn't occur, so it seems to be something specific to the relude reducer implementation.

https://reasonml.github.io/try?rrjsx=true&reason=NoAQRgzgdAxg9gOwGYEsDmACA3gKwgDwC4MBmAXwF0BuAKBoBcBPABwFMMJ6BDe9gXmzwArgnrEUosrQYt2XGPRSIMfGhgwAfDAEkEMNZowARVjGkAbVvQwAnVgBMhMVjZUYAFJx6sANBnmKiACUKgB8BhAA7ij0MAAWHgFKCCFYBlq6MGEG6gBS0OZwaO4ARJklQbTq6ljCosRevLBwItYA1BgAjFJ06lomWXzh1Rj5UIXFJQMVVdW1LfUc3E111gC0XT3qPTQAtnCOlhgAwgu8rgJp6qB2Ac27zIisohQGlta7XADW-B4AfgguLtWCEhtgchh3h5Gr4OM97KCMAAlVh3IQQVgoxzOGzuOzYlx+eatYgABjIlV61QAPPYUAA3YYjDDU5hM5ngjkjFF3Tg2CTFCFcjCA4EYNodEoYGDmFAwH72DBSiVLfkINAAfTgSA1EnonmWrGarRCKqlimBEBKPiFzKCtu2tupAHo2U6wEJ6PRlIhjrL5XwsBqwnCEPZ3JkgmRQtgeQooHyBaVMnZgaIKmQWc6PV7EOyaTnvQgML7-V9A8GwRiw+4BlGY1g4-QE-Q1ZMBqnnvQM1nC3mIS66YzZj0dk2jAB5ACyKKgdjDLgAKnAAKKWNP0ADqMTi2nD1NOrRcIqBrD4JUPohcUudoT8JWYdnpKFYkRmNCAA

I put a small example in this repo for debugging. Run npm run watch in one terminal, and npm run sandbox in another to see it

This is more a note for myself, but it would be worth seeing if the same behavior happens in bloodyowl/reason-react-update, and if not, check for implementation differences with our lib.

This is more a note for myself, but it would be worth seeing if the same behavior happens in bloodyowl/reason-react-update, and if not, check for implementation differences with our lib.

Hi @andywhite37 , I tried on my branch and the same behavior happens in bloodyowl/reason-react-update:

screenshot_2019-12-29 22-42-29

There could be multiple reducer if reducer is not pure.

I'm able to reproduce the same behavior in React.useReducer API.
Please see demo (line 25 ~ 28).

Found an issue may be related.

#23 fix released in v0.8.1

I've been running our app with v0.8.1, and I think I'm actually experiencing some issues with the fix. I haven't nailed down exactly what's going on, but our code works with v0.8.0 and not with v0.8.1, so I suspect there's something afoot.

My suspicion is that the useRef is creating some kind of unwanted closure or memoization of props (don't quote me on that), which is causing new prop inputs to not be seen, or something like that. I'm seeing issues where I'm trying to render a component with new props, but the new props are not making it through to the reducer function (closure maybe?) anymore.

@mlms13 - found a tweet from Dan Abramov that says you shouldn't do effects in useReducer and other functions like useState, because they can run twice, so maybe this issue is just a known behavior. We don't do any effects in our reducer functiosn - we either do the SideEffect or IO-based actions, so we haven't really run into this double-effect issue ourselves.

https://twitter.com/dan_abramov/status/1102631338853715973

I don't think I'll do anything right at the moment, but we may want to consider making this useRef behavior configurable (like an option to wrap your reducer or not), or possibly reverting the change. I would like to keep the tests and stuff in place, even if we change the assertions to check the "undesirable" but possibly expected behavior.

Anyone have any thoughts?

CC @jihchi @gaku-sei @mlms13

v0.8.2 removed the useRef wrapper, so the reducer now works how it did when this issue was originally reported. In other words, unmanaged side effects in the reducer function may run more than one time, so you should avoid effects in the reducer, or use the controlled effects that we support with SideEffect or IO updates.