pureFinalPropsSelectorFactory memoizes wrongly if mapStateToProps/mapDispatchToProps throws

[MrWolfZ]

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

bug

What is the current behavior?

The pureFinalPropsSelectorFactory memoizes results of calls to the mapStateToProps/mapDispatchToProps functions. However, if either of those functions throws, then the memoization will memoize the inputs, but not the outputs. On subsequent calls to the pureFinalPropsSelector with the same props, it will erroneously return the result of the last successful call.

The simplest way to see this bug in action is to duplicate the line

expect(() => store.dispatch({ type: 'APPEND', body: 'a' })).toThrow()

in the test should not swallow errors when bailing out early in connect.spec.js (also change the condition in mapState to state === 'a' || state === 'aa'. With those changes the test will fail, even though it shouldn't.

EDIT: Sorry, I think the reproduction was wrong. I'll come up with a better way to show the issue.

What is the expected behavior?

When mapStateToProps or mapDispatchToProps throws, then either the error should be memoized and thrown again, or nothing should be memoized from the erroneous call.

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

I've only checked this on the v7-beta branch with React 16.8, but I expect this issue exists for some time already.

[MrWolfZ]

Alright, I wrote a dedicated test that shows the issue. However, please note that I was not able to find a way to trigger this behaviour through the public API (which of course doesn't mean it isn't possible, but this issue is probably not very critical).

So, here the test (in a new file selectorFactory.spec.js):

import { pureFinalPropsSelectorFactory } from '../../src/connect/selectorFactory'

describe('selectorFactory', () => {
  describe(pureFinalPropsSelectorFactory.name, () => {
    describe('resulting pureFinalPropsSelector', () => {
      it('should not memoize outdated inputs when mapState throws', () => {
        const selector = pureFinalPropsSelectorFactory(
          s => {
            if (s === 1) {
              throw new Error()
            }

            return { state: s }
          },
          () => ({}),
          (a, b, c) => ({ ...a, ...b, ...c }),
          () => void 0,
          {
            areStatesEqual: (a, b) => a === b,
            areOwnPropsEqual: (a, b) => a === b,
            areStatePropsEqual: (a, b) => a === b
          }
        )

        const ownProps = { a: 'a' }
        selector(0, ownProps)
        expect(() => selector(1, ownProps)).toThrow()
        expect(() => selector(1, ownProps)).toThrow()
      })
    })
  })
})

[markerikson]

Thanks, I'll try to look at this a bit later.

FWIW, off the top of my head, the issue probably doesn't surface to the external code because we should always be throwing the error from the wrapper component once it's caught, thus causing this component to unmount.

But yeah, worth fixing.

[markerikson]

I'm going to close this as a WONTFIX for now, largely because we're encouraging folks to stop using connect (even though it'll be fully supported in v8).

If someone still feels strongly about this please comment, or even better file a PR to improve behavior here.