Reducer gets called twice while dispatch is called once

[ghivert]

Hi!

I’m starting with React Basic Hooks those days for a Firebase project for my company and I find the package really amazing.
But I’m struggling with a specific point (not a really blocking point btw) : I’m using a Reducer and a Context at a root level component in order to require it from anywhere, and it’s working great. I’m using Firebase Authentication and I’m listening to the authentication which signals the application when a user logs in or out to ping the store and update it accordingly. But strangely, for the user auth, the reducer gets called twice while the dispatch is called once.

Here’s a Minimal Reproducible Example:

-- Main.purs
module Main where

import Prelude

import Data.Maybe (Maybe(..), maybe)
import Data.Nullable (Nullable)
import Data.Nullable as Nullable
import Effect (Effect)
import Effect.Class.Console as Console
import Effect.Exception (throw)
import Effect.Unsafe (unsafePerformEffect)
import React.Basic.DOM (render)
import React.Basic.DOM as DOM
import React.Basic.Events (handler_)
import React.Basic.Hooks (type (/\), Component, ReactContext, component, useEffectOnce, useReducer, (/\))
import React.Basic.Hooks as RB
import React.Basic.Hooks as React
import Web.DOM.NonElementParentNode (getElementById)
import Web.HTML as HTML
import Web.HTML.HTMLDocument (toNonElementParentNode)
import Web.HTML.Window as Window

type User = { uid :: String }

foreign import foreignAuthStateChanged :: (Nullable User -> Unit) -> Effect (Effect Unit)
foreign import foreignSignInWithPopup :: String -> Array String -> Effect Unit
foreign import foreignSignOut :: Effect Unit

type State = { user :: Maybe User}
data Action
  = UpdateUser (Maybe User)

init :: State
init = { user: Nothing }

reduce :: State -> Action -> State
reduce state (UpdateUser user) =
  let _ = unsafePerformEffect $ Console.warn "UpdateUser" in
  state { user = user }

onAuthStateChanged :: (Maybe User -> Effect Unit) -> Effect (Effect Unit)
onAuthStateChanged callback =
  foreignAuthStateChanged $ Nullable.toMaybe >>> callback >>> unsafePerformEffect

context :: ReactContext (State /\ (Action -> Effect Unit))
context =
  unsafePerformEffect
  $ RB.createContext
  $ init /\ (const $ Console.warn "Nothing implemented")

mkApp :: Component Unit
mkApp = do
  component "App" \props -> React.do
    store <- useReducer init reduce
    let state /\ dispatch = store
    useEffectOnce $ onAuthStateChanged $ \user -> do
      Console.warn "Dispatching"
      dispatch $ UpdateUser user
    pure $ RB.provider context store
      [ DOM.pre_ [DOM.text $ show state.user]
      , DOM.button
        { onClick: handler_ $ maybe (foreignSignInWithPopup "Google" ["openid"]) (const foreignSignOut) state.user
        , children:
          [ DOM.text $ maybe "Connect" (const "Disconnect") state.user]
        }
      ]

main :: Effect Unit
main = do
  document <- Window.document =<< HTML.window
  container <- getElementById "app" $ toNonElementParentNode document
  case container of
    Nothing -> throw "No Element \"app\" found."
    Just node -> do
      root <- mkApp
      render (root unit) node

// Main.js
exports.foreignAuthStateChanged = callback => () => {
  return firebase.auth().onAuthStateChanged(callback)
}

const selectProvider = provider => {
  switch (provider) {
    case 'Google':
      return new firebase.auth.GoogleAuthProvider()
  }
}

exports.foreignSignInWithPopup = provider => scopes => () => {
  const authProvider = selectProvider(provider)
  scopes.forEach(scope => authProvider.addScope(scope))
  return firebase.auth().signInWithPopup(authProvider)
}

exports.foreignSignOut = () => {
  return firebase.auth().signOut()
}

When running this, we got:

Is there anything I’m doing wrong or is it a React problem? I saw that React.StrictMode sometimes calls the reducer twice, but I don’t use it… Because everything is pure it’s not so painful, but I don’t understand what’s happening.
For information, connect / disconnect just run the reducer once after the first time. It’s really only the first time. I also tried it in pure React and couldn’t reproduce the bug.
I created a repo with the example in order to test it easily: ghivert/example-bug-purs.

[maddie927]

Hmm, I haven't looked at your demo yet, but do you see the same behavior using the production version of React?

[maddie927]

The example seems to require a service running on port 5000 that isn't included.

[maddie927]

Found it: https://github.com/facebook/react/blob/30b47103d4354d9187dc0f1fb804855a5208ca9f/packages/react-reconciler/src/ReactFiberHooks.new.js#L767-L774

It looks like React sometimes runs the reducer immediately when dispatch is called and this code avoids re-running the reducer later if the reducer hasn't changed. We always hit the 'else' because the FFI in the PS useReducer hook implementation wraps it using mkFn2 each time it's passed into React's implementation of useReducer. This means from React's perspective it's a new reducer function passed in on every render. Doing this in JS would likely result in the same behavior:

function App() {
  const ... = useReducer((state, action) => state, init)
}

as opposed to

const reducer = (state, action) => state
function App() {
  const ... = useReducer(reducer, init)
}

Since the reducer is a pure function, I think the only implication here is performance. Not sure how to fix it though without making some kind of effectful mkReducer :: (state -> action -> state) -> Effect (Reducer state action) where newtype Reducer state action = Reducer (Fn2 state action state). Might be worth it though, both for performance and to reduce (lol) confusion?

[maddie927]

The "eager" call is to decide whether a re-render needs to be queued at all: https://github.com/facebook/react/blob/30b47103d4354d9187dc0f1fb804855a5208ca9f/packages/react-reconciler/src/ReactFiberHooks.new.js#L1706

[maddie927]

What do you think of this implementation for the next release?

newtype Reducer state action
  = Reducer (Fn2 state action state)

-- | Creating reducer functions for React is effectful because
-- | React uses the function instance's reference to optimise
-- | rendering behavior.
mkReducer :: forall state action. (state -> action -> state) -> Effect (Reducer state action)
mkReducer = pure <<< Reducer <<< mkFn2

-- | Use `mkReducer` to construct a reducer function.
useReducer ::
  forall state action.
  state ->
  Reducer state action ->
  Hook (UseReducer state action) (state /\ (action -> Effect Unit))
useReducer initialState (Reducer reducer) =
  unsafeHook do
    runEffectFn3 useReducer_
      (mkFn2 Tuple)
      reducer
      initialState

You could consume it like this:

reduce :: State -> Action -> State
reduce state (UpdateUser user) =
  state { user = user }

mkApp :: Component Unit
mkApp = do
  reducer <- mkReducer reduce
  component "App" \props -> React.do
    state /\ dispatch <- useReducer init reducer
    ...

Or this:

reduce :: Reducer State Action
reduce =
  unsafePerformEffect do
    mkReducer \state (UpdateUser user) ->
      state { user = user }

mkApp :: Component Unit
mkApp = do
  component "App" \props -> React.do
    state /\ dispatch <- useReducer init reduce
    ...

[ghivert]

Thanks for a so quick response!

I’m a torn between two things: I think that useReducer in the way it is now is fine because more directly related to vanilla React, without adding extra type, and so reduce complexity when jumping in. But I also think it’s important to have a performance-friendly API, for library maintainers at least. So maybe the best of two worlds is to have both useReducer and useReducer'? The first would act like the one now, and the second one like what you proposed.

I’m wondering if the mkReducer should be effectful or not. Because, I feel like it’s not a magic way to get a reducer. Correct me if I’m wrong, but this:

reduce :: State -> Action -> State
reduce state (UpdateUser user) =
  state { user = user }

mkApp :: Component Unit
mkApp = do
  reducer <- mkReducer reduce
  component "App" \props -> React.do
    state /\ dispatch <- useReducer init reducer
    ...

Would be compiled to something like this in React:

const reduce = state => action => {
  // Do reducer stuff.
}

const App = () => {
  const reducer = (function(f) { 
    return function(a, b) { 
      return f(a)(b)
    }
  })(reduce)
  const [state, dispatch] = useReducer(init, reducer)
  ...
}  

Which in the end generates a new function at each render? The other option with unsafePerformEffect seems to solve the problem effectively, but we have to do an unsafePerformEffect… 😕 If the reducer is not a the top-level, it will be compiled to another function. But with the effect, we could still do something like:

reduce :: Effect (Reducer State Action)
reduce = mkReducer $ 
  \state (UpdateUser user) ->
    state { user = user }

mkApp :: Component Unit
mkApp = do
  reducer <- reduce
  component "App" \props -> React.do
    state /\ dispatch <- useReducer init reducer
    ...

So maybe just mkReducer :: forall state action. (state -> action -> state) -> Reducer state action is enough? Because in the end, all we want is to define a function at the top-level which will not be regenerated at each render?

[maddie927]

I’m wondering if the mkReducer should be effectful or not.

Definitely. Anything that deals with "instances" and reference-based equality is effectful. It's a concept mostly absent from PureScript, but it occasionally pops up during JS interop like this. component is effectful for the same reason. React uses the function instance as the "identity" or "key" of the component. If you tried to create a component inside a render function, it wouldn't matter if it contained the same logic or rendered the same JSX on every render, React would still discard the entire tree below that component, including its state, and rebuild it. This is because you could have changed the implementation. There's no way for React to know.

It's easier to see with objects. In PS, regular functions are pure, i.e. the same input always gives the same output.

let
  createObj _ = { a: 1 }
  o1 = createObj unit
  o2 = createObj unit

o1 == o2 -- true, PS assumes value-level equality

const createObj = () => ({ a: 1 })
const o1 = createObj()
const o2 = createObj()

o1 == o2 // false, JS assumes reference equality

This means that the difference in assumptions, not the code itself, is what makes the JS version effectful. You could of course use some kind of deepEqual in JS or unsafeRefEq in PS, but those are edge cases. You also can't use eq (PS) or deepEqual (JS) on a function to determine if its implementation is different from another function.

So, the JS implementation forces us to care about "function instances", and that concept isn't compatible with "pure PS functions", so we use a couple tricks to interact with it in a safer way from PS:

1. If any code uses reference equality (or mutation, global values, etc) we put it in an Effect
2. We then wrap the result in a newtype to separate it conceptually from the regular version of the value, in this case a Fn2 and only export the effectful wrapper around the constructor.

Some more things that are effectful for the same reason: Effect.Ref.new, React.Basic.createContext, useRef & useState (hooks are effects), Data.Array.ST.thaw & Record.ST.thaw (these clone the object, but it's effectful because the clone is intended to be used as a mutable instance)

---

Would be compiled to something like this in React

Almost:

const reduce = state => user =>
  ({ user, ...state })

// mkApp :: Component Unit
const mkApp = () => {
  const reducer = mkReducer(reduce)()
  return component("App")(props => {
    const [state, dispatch] = useReducer(init)(reducer)()
    ...
  })()
}

mkApp is an effect (function/thunk when compiled to JS) that produces a component instance. In this case I did end up skipping the newtype for the component function specifically. Component Unit is just an alias for Effect (Unit -> JSX). The component instance is hidden inside that Unit -> JSX function, because it pre-applies element. What matters in this example is that reducer is defined outside the render function, whether that's at the module level using unsafePerformEffect or during the component creation effect within mkApp.

So why not make mkReducer Effect (state -> action -> state) (or change Reducer to a simple type alias, same thing)? Because from useReducer's perspective the result of mkReducer and a normal PS state -> action -> state do not behave the same. One works correctly and the other exhibits this bug. component doesn't have this issue because using a Int -> JSX function created using component vs a regular PS Int -> JSX function are both valid from the parent component's perspective. The difference in behavior (having or not having state) is an implementation detail.

[ghivert]

Sorry for the delay.

Thank you for your answer, I see your point now. I didn’t get that Effectful functions was there for React to be able to compare instances, that was a point I was wondering about how React Basic was compiling the code to React, but it’s really much clearer.

I continued to work on my project and the reducer bug happens also for different parts of the reducer, like in the routing. So I think it’s better in any case to force the users to use mkReducer. This way we can ensure the users will not have this bug in dev or production.

---

On an other topic, I find React Basic Hooks really amazing, so thanks for the hard work in overall.

[maddie927]

Thanks! c:

[ghivert]

I just wanted to say that I tried the new version (6.0.0) and it’s working for me: the duplication bug go away!

Thank you very much again for such a quick update!

[maddie927]

I think mkReducer may have been a mistake. It makes it nearly impossible to access props from within the reducer function. Trying to decide if I should undo it and add some documentation for getting around the duplicate calls or if the "logically correct but slightly less efficient" way it worked before was even that bad. It's still possible to get around the performance issue in edge cases using things like useMemo.