From da16f134fdefda10abebd7200842e179e359acb0 Mon Sep 17 00:00:00 2001 From: Andy White Date: Wed, 5 Feb 2020 21:38:11 -0700 Subject: [PATCH] Add useRef flag to useReducer to conditionally enable useRef (default is false) --- __tests__/ReludeReact_Reducer_test.re | 101 +++++++++++ __tests__/ReludeReact_Render_test.re | 12 ++ __tests__/ReludeReact_test.re | 59 ------- src/ReludeReact_Reducer.re | 230 ++++++++++++++------------ 4 files changed, 240 insertions(+), 162 deletions(-) create mode 100644 __tests__/ReludeReact_Reducer_test.re create mode 100644 __tests__/ReludeReact_Render_test.re delete mode 100644 __tests__/ReludeReact_test.re diff --git a/__tests__/ReludeReact_Reducer_test.re b/__tests__/ReludeReact_Reducer_test.re new file mode 100644 index 0000000..6e4de1d --- /dev/null +++ b/__tests__/ReludeReact_Reducer_test.re @@ -0,0 +1,101 @@ +open Jest; +open Expect; +open ReactTestingLibrary; + +// This component tests the behavior of useReducer(~useRef=false, ...) +// false is the default, so the useRef parameter is omitted. +module Counter = { + type state = int; + + let clickCount = ref(0); // tracks updates as plain old side effects + + let initialState = 0; + + type action = + | Inc; + + let reducer: ReludeReact.Reducer.reducer(action, state) = + (state: state, action: action) => { + switch (action) { + | Inc => + // Note: This is not how effects are meant to be done, this is just testing some + // react behavior + clickCount := clickCount^ + 1; + Update(state + 1); + }; + }; + + [@react.component] + let make = () => { + let (state, send) = + ReludeReact_Reducer.useReducer(reducer, initialState); + +
+ {React.string("Count: " ++ string_of_int(state))} + +
; + }; +}; + +// This component tests the behavior of useReducer(~useRef=true, ...) +module CounterWithRef = { + type state = int; + + let clickCount = ref(0); // tracks updates as plain old side effects + + let initialState = 0; + + type action = + | Inc; + + let reducer: ReludeReact.Reducer.reducer(action, state) = + (state: state, action: action) => { + switch (action) { + | Inc => + // Note: This is not how effects are meant to be done, this is just testing some + // react behavior + clickCount := clickCount^ + 1; + Update(state + 1); + }; + }; + + [@react.component] + let make = () => { + let (state, send) = + ReludeReact_Reducer.useReducer(~useRef=true, reducer, initialState); + +
+ {React.string("Count: " ++ string_of_int(state))} + +
; + }; +}; + +describe("ReludeReact_Reducer", () => { + // This tests the current "expected" behavior of useReducer in terms of how it handles + // uncontrolled side effects. The expected behavior is that uncontrolled side effects might + // run multiple times (in this case 2). This behavior is maybe not desired, but according to + // a casual tweet by Dan Abramov, it should be expected. Overall, it's discouraged to have + // unmanaged side effects in the reducer, so this is not something we would normally run into + // if we're using the Relude SideEffect and IO-based update commands. + test("Reducer should be called twice when not using ref", () => { + let renderResult = render(); + renderResult |> getByText(~matcher=`Str("Increment")) |> FireEvent.click; + expect(Counter.clickCount^) |> toEqual(2); + }); + + // This is testing our experimental "useRef" option for the + // ReludeReact.Reducer.useReducer. This prevents unmanaged side effects from + // running more than once, but might cause some other unrelated problems, like + // with unwanted closure/memoization of input props. These other problems have not + // yet be fully diagnosed. + test("Reducer should be called once when using ref wrapper", () => { + let renderResult = render(); + renderResult |> getByText(~matcher=`Str("Increment")) |> FireEvent.click; + expect(CounterWithRef.clickCount^) |> toEqual(1); + }); +}); \ No newline at end of file diff --git a/__tests__/ReludeReact_Render_test.re b/__tests__/ReludeReact_Render_test.re new file mode 100644 index 0000000..60fc66a --- /dev/null +++ b/__tests__/ReludeReact_Render_test.re @@ -0,0 +1,12 @@ +open Jest; +open Expect; + +describe("ReludeReact_Render", () => { + test("Render.ifTrue true", () => { + expect(ReludeReact_Render.ifTrue(
, true)) |> toEqual(
) + }); + + test("Render.ifTrue false", () => { + expect(ReludeReact_Render.ifTrue(
, false)) |> toEqual(React.null) + }); +}); \ No newline at end of file diff --git a/__tests__/ReludeReact_test.re b/__tests__/ReludeReact_test.re deleted file mode 100644 index d0aa927..0000000 --- a/__tests__/ReludeReact_test.re +++ /dev/null @@ -1,59 +0,0 @@ -open Jest; -open Expect; -open ReactTestingLibrary; - -module Counter = { - type state = int; - - let initialState = 0; - - type action = - | Inc; - - let reducer: ReludeReact.Reducer.reducer(action, state) = - (state: state, action: action) => { - switch (action) { - | Inc => - Js.log("Inc"); - Update(state + 1); - }; - }; - - [@react.component] - let make = () => { - let (state, send) = - ReludeReact_Reducer.useReducer(reducer, initialState); - -
- {React.string("Count: " ++ string_of_int(state))} - -
; - }; -}; - -describe("ReludeReact", () => { - test("one", () => - pass - ); - test("two", () => - expect(true) |> toBe(true) - ); - - test("three", () => { - expect(ReludeReact_Render.ifTrue(
, false)) |> toEqual(React.null) - }); - - test("Reducer should be called once", () => { - let wrapper = render(); - - let _ = [%raw {|jest.spyOn(console, 'log').mockImplementation(() => {})|}]; - let _ = [%raw {|expect(console.log).toHaveBeenCalledTimes(0)|}]; - - wrapper |> getByText(~matcher=`Str("Increment")) |> FireEvent.click; - - let _ = [%raw {|expect(console.log).toHaveBeenCalledTimes(1)|}]; - pass; - }); -}); \ No newline at end of file diff --git a/src/ReludeReact_Reducer.re b/src/ReludeReact_Reducer.re index f905309..65fe4f6 100644 --- a/src/ReludeReact_Reducer.re +++ b/src/ReludeReact_Reducer.re @@ -48,117 +48,141 @@ type stateAndSideEffects('action, 'state) = { sideEffects: ref(array(SideEffect.t('action, 'state))), }; -let useReducer = (reducer: reducer('action, 'state), initialState: 'state) => { - let refReducer = - React.useRef(({state, sideEffects} as stateAndSideEffects, action) => { - let update = reducer(state, action); - - switch (update) { - | NoUpdate => stateAndSideEffects - - | Update(state) => {...stateAndSideEffects, state} - - | UpdateWithSideEffect(state, sideEffect) => { - state, - sideEffects: - ref( - Belt.Array.concat( - sideEffects^, - [|SideEffect.Uncancelable.lift(sideEffect)|], - ), +let useReducer = + ( + ~useRef: bool=false, + reducer: reducer('action, 'state), + initialState: 'state, + ) => { + // This wraps the given reducer function with the ability to capture the side effects + // emitted by the various types of updates, and stick them in our mutable array of effects to run later + let reducerWithSideEffects = + ({state, sideEffects} as stateAndSideEffects, action) => { + let update = reducer(state, action); + + switch (update) { + | NoUpdate => stateAndSideEffects + + | Update(state) => {...stateAndSideEffects, state} + + | UpdateWithSideEffect(state, sideEffect) => { + state, + sideEffects: + ref( + Belt.Array.concat( + sideEffects^, + [|SideEffect.Uncancelable.lift(sideEffect)|], ), - } - - | UpdateWithCancelableSideEffect(state, cancelableSideEffect) => { - state, - sideEffects: - ref( - Belt.Array.concat( - sideEffects^, - [|SideEffect.Cancelable.lift(cancelableSideEffect)|], - ), + ), + } + + | UpdateWithCancelableSideEffect(state, cancelableSideEffect) => { + state, + sideEffects: + ref( + Belt.Array.concat( + sideEffects^, + [|SideEffect.Cancelable.lift(cancelableSideEffect)|], ), - } - - | SideEffect(uncancelableSideEffect) => { - ...stateAndSideEffects, - sideEffects: - ref( - Belt.Array.concat( - stateAndSideEffects.sideEffects^, - [|SideEffect.Uncancelable.lift(uncancelableSideEffect)|], - ), + ), + } + + | SideEffect(uncancelableSideEffect) => { + ...stateAndSideEffects, + sideEffects: + ref( + Belt.Array.concat( + stateAndSideEffects.sideEffects^, + [|SideEffect.Uncancelable.lift(uncancelableSideEffect)|], ), - } - - | CancelableSideEffect(cancelableSideEffect) => { - ...stateAndSideEffects, - sideEffects: - ref( - Belt.Array.concat( - stateAndSideEffects.sideEffects^, - [|SideEffect.Cancelable.lift(cancelableSideEffect)|], - ), + ), + } + + | CancelableSideEffect(cancelableSideEffect) => { + ...stateAndSideEffects, + sideEffects: + ref( + Belt.Array.concat( + stateAndSideEffects.sideEffects^, + [|SideEffect.Cancelable.lift(cancelableSideEffect)|], ), + ), + } + + | UpdateWithIO(state, ioAction) => + // The IO must have an 'action type for both the success and error channels - this + // way we know that the errors have been properly handled and translated to the appropriate action. + // Run the IO to get the success and error actions, then just send them. + // TODO: we don't have cancelable IOs (yet?) + let sideEffect: SideEffect.t('action, 'state) = ( + context => { + ioAction + |> Relude.IO.unsafeRunAsync( + fun + | Ok(action) => context.send(action) + | Error(action) => context.send(action), + ); + None; } - - | UpdateWithIO(state, ioAction) => - // The IO must have an 'action type for both the success and error channels - this - // way we know that the errors have been properly handled and translated to the appropriate action. - // Run the IO to get the success and error actions, then just send them. - // TODO: we don't have cancelable IOs (yet?) - let sideEffect: SideEffect.t('action, 'state) = ( - context => { - ioAction - |> Relude.IO.unsafeRunAsync( - fun - | Ok(action) => context.send(action) - | Error(action) => context.send(action), - ); - None; - } - ); - { - state, - sideEffects: - ref( - Belt.Array.concat( - stateAndSideEffects.sideEffects^, - [|sideEffect|], - ), - ), - }; - - | IO(ioAction) => - let sideEffect: SideEffect.t('action, 'state) = ( - context => { - ioAction - |> Relude.IO.unsafeRunAsync( - fun - | Ok(action) => context.send(action) - | Error(action) => context.send(action), - ); - None; - } - ); - { - ...stateAndSideEffects, - sideEffects: - ref( - Belt.Array.concat( - stateAndSideEffects.sideEffects^, - [|sideEffect|], - ), + ); + { + state, + sideEffects: + ref( + Belt.Array.concat( + stateAndSideEffects.sideEffects^, + [|sideEffect|], ), - }; + ), }; - }); + | IO(ioAction) => + let sideEffect: SideEffect.t('action, 'state) = ( + context => { + ioAction + |> Relude.IO.unsafeRunAsync( + fun + | Ok(action) => context.send(action) + | Error(action) => context.send(action), + ); + None; + } + ); + { + ...stateAndSideEffects, + sideEffects: + ref( + Belt.Array.concat( + stateAndSideEffects.sideEffects^, + [|sideEffect|], + ), + ), + }; + }; + }; + + // This takes the given initial state, and adds a mutable array of side effects which we capture in the reduce + // function, and then execute later using a normal React useEffect hook. + let initialStateWithSideEffects = { + state: initialState, + sideEffects: ref([||]), + }; + + // See https://github.com/reazen/relude-reason-react/issues/21 + // We're experimenting with the ability to wrap the reducer function in a ref to prevent side effects from + // running multiple times. However, side effects are discouraged in reducers, and the best practice with this + // library is to manage side effects using the SideEffect or IO-based updates. Also, wrapping the reducer + // in a ref like this seems to cause some yet-to-be-diagnosed problems with prop updates. let ({state, sideEffects}, send) = - React.useReducer( - refReducer |> React.Ref.current, - {state: initialState, sideEffects: ref([||])}, - ); + if (useRef) { + let reducerWithSideEffectsRef = React.useRef(reducerWithSideEffects); + React.useReducer( + reducerWithSideEffectsRef |> React.Ref.current, + initialStateWithSideEffects, + ); + } else { + React.useReducer(reducerWithSideEffects, initialStateWithSideEffects); + }; // This registers the side effects that were emitted by the reducer in a react effect hook. // When the hook runs, it will execute all the side effects and will @@ -185,4 +209,4 @@ let useReducer = (reducer: reducer('action, 'state), initialState: 'state) => { // Finally, we return our initial state, and the send function for use in the component (state, send); -}; +}; \ No newline at end of file