facebookarchive / redux-react-hook

React Hook for accessing state and dispatch from a Redux store
MIT License
2.16k stars 103 forks source link

Init prevArg with an empty object #95

Closed pauldijou closed 3 years ago

pauldijou commented 4 years ago

As mentioned in #94 , this will prevent issues if the initial Redux state is undefined, which would result in prevArg === arg, skipping the first call to fn.

ianobermiller commented 4 years ago

I should have mentioned this on the last PR. Could you please add a test for this and the last fix?

pauldijou commented 4 years ago

I added a test for that fix, but so far, I failed to craft one for the previous fix.

While working on tests, I noticed that some test are using the render function helper (which wrap the content inside a StoreProvider with the global store) while still giving a StoreProvider with a store (either local or the global one) on the test, resulting in 2 nested StoreProvider. It works since I guess consumers will stop at the 1st provider they found, but I'm not sure that's what the test wanted.

Bonus point, here is my last try for the previous text fix, I could not make error boundaries properly work on that test and I fail to know why so far.

it('should not memoize argument if the mapper function throw', () => {
    const store = createReduxStore((state: number = 0, action: any) =>
      action.type === 'test' ? action.payload : state,
    );

    class Boundary extends React.Component<{}, {hasError: boolean}> {
      state = {hasError: false};

      static getDerivedStateFromError() {
        console.log('getDerivedStateFromError');
        return {hasError: true};
      }

      componentDidCatch() {
        console.log('componentDidCatch');
      }

      render() {
        console.log('render', this.state.hasError);
        return (
          <div>
            {this.state.hasError ? 'error:' : 'no error:'}
            {this.props.children}
          </div>
        );
      }
    }

    const mapState = (s: number) => {
      if (s > 0) {
        throw new Error('Must be zero');
      } else {
        return s;
      }
    };

    const Component = () => {
      const bar = useMappedState(mapState);
      function handleClick() {
        store.dispatch({type: 'test', payload: 1});
      }
      return (
        <div>
          {bar}
          <button onClick={handleClick}></button>
        </div>
      );
    };

    act(() => {
      ReactDOM.render(
        <StoreContext.Provider value={store}>
          <Boundary>
            <Component />
          </Boundary>
        </StoreContext.Provider>,
        reactRoot,
      );
    });

    const button = reactRoot.querySelector('button');

    expect(store.getState()).toBe(0);
    expect(getText()).toBe('no error:0');

    act(() => {
      button &&
        button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
    });

    expect(store.getState()).toBe(1);
    expect(getText()).toBe('error:0');
  });
ianobermiller commented 3 years ago

Don't worry too much about a test for the previous fix. Thanks for adding a test for this one!