connect swallows errors in mapStateToProps (new bug in 8.0.0)

[grumd]

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

• React: 18.2
• ReactDOM/React Native: react-dom 18.2
• Redux: 4.2.0
• React Redux: 8.0.2

What is the current behavior?

https://codesandbox.io/s/gifted-architecture-wr4f86?file=/src/App.js

1. Have a code that can cause an error in mapStateToProps, e.g.

const mapStateToProps = (state) => {
  return {
    foo: state.foo.length, // when foo is undefined, causes an error
  };
};

2. Dispatch an action that changes the state so that it causes the error above

dispatch(updateFoo(undefined));

3. Rerender doesn't happen, no errors emitted in console, componentDidCatch catches nothing. The error is swallowed.

What is the expected behavior?

This issue makes debugging code that's using react-redux really hard.
The expected behaviour is that the error is visible in console.
It worked fine in 7.2.8: https://codesandbox.io/s/charming-firefly-coekps

Which browser and OS are affected by this issue?

Windows, latest Chrome here. Probably affects all browsers.

Did this work in previous versions of React Redux?

• Yes

[grumd]

Just tested - it worked fine in 7.2.8

Here's a 7.2.8 fork https://codesandbox.io/s/charming-firefly-coekps

Stopped working in 8.0.0 alpha.

[markerikson]

This is likely due to the switch to useSyncExternalStore, which uses the same approach that useSelector does in trapping errors.

I'd have to investigate at some point, but I'm not sure how much we'll be able to do here, and any further changes to connect are really low priority.

Honestly the main advice here would be to not use connect, and switch to useSelector instead.

[grumd]

React 18 still supports class components, and you'll need react-redux 8 for React 18.
It's not the biggest blocking bug of course, but still would be nice if react-redux wasn't preventing people from upgrading to React 18 when their codebase has legacy class components.

I understand your point though. Would be great if you could take a look someday! Thank you for your work.

[markerikson]

Sure. To be clear, I wouldn't see this as "blocking an upgrade" to React-Redux v8 + React 18. We actually are on both in https://github.com/replayio/devtools right now, and we do have a bunch of connected class components in our codebase still. So, I know what that's like :) It's "just" that if there's an error in mapState it's not getting logged. Totally get that that's a pain, and I'm not trying to diminish that aspect - just noting that in terms of "does the code run and do the libs work", the answer is definitely "yes".

[grumd]

Yeah I completely agree, don't get me wrong. Just leaving a little bit of hope that maybe you'll be bored one day and will fix it anyway :)

[itsjohncs]

Just ran into this in 7.2.5 ([email protected], [email protected], and [email protected]). So the story of which versions are affected seems to be more muddled.

Edit: I was encountering something else. Selectors are called in various places by either react-redux, react, or redux (I've had trouble figuring out where this code is from...). It seems that in some of those places, errors are suppressed. But when the selector is called during rendering, errors are propagated correctly in my setup.

[rdonnelly]

I think we're running into something similar in our application. When we hit an error in a selector, the exception seems to get gobbled up here:

react-redux/src/components/connect.tsx

Lines 131 to 141 in 8cf538c

	try {
	// Actually run the selector with the most recent store state and wrapper props
	// to determine what the child props should be
	newChildProps = childPropsSelector(
	latestStoreState,
	lastWrapperProps.current
	)
	} catch (e) {
	error = e
	lastThrownError = e as Error | null
	}

We were previously relying on that thrown error to bubble up and get caught by an error handler, but now our app won't re-render and won't show an error. I won't claim that this is the best pattern (the application code is pretty old at this point), but is there any way to let that error pass through? Or if we did switch to useSelector syntax rather than connect, would that alleviate our issues?

[markerikson]

@rdonnelly : afraid I don't have the brain bandwidth to answer this one further atm. The logic in connect is frankly very fragile, doubly so in v8, and we're not going to make further changes to it. I'd suggest trying out useSelector and see what happens.

[alexanderchr]

Just spent hours trying to figure out why my app was silently crashing and this was the reason. In my opinion this is a quite significant regression, at least enough for me to have to downgrade to 7.1.x for now. Clearly useSelector is the way to go but I cannot justify removing all connects right now.

I really wish you'd reconsider the decision to not make further changes to connect, but understand it might not be an option. At the very minimum it should be documented because it is a breaking change from 7.1.x even if it does not break any happy paths.

[alexanderchr]

Just a thought, would it be possible to log the error here if NODE_ENV !== 'production'?

react-redux/src/components/connect.tsx

Lines 131 to 141 in 8cf538c

	try {
	// Actually run the selector with the most recent store state and wrapper props
	// to determine what the child props should be
	newChildProps = childPropsSelector(
	latestStoreState,
	lastWrapperProps.current
	)
	} catch (e) {
	error = e
	lastThrownError = e as Error | null
	}

Would still be breaking for anyone whose logic depends upon errors in selectors bubbling up but hopefully that's not very many. Not perfect but a minimal change that would at least make it easier to find the cause of silent crashes caused by this during development.

[markerikson]

@alexanderchr I'm open to PRs to improve this, but right now all the time and mental bandwidth I can scrape together for Redux work is focused on RTK 2.0 and the related major versions, so I don't have the energy to think about this one myself.

(FWIW, I agree that changes in behavior between versions can be considered "breaking". But we never specifically documented or specified how connect behaves with regards to errors in selectors as part of our public known API, it's always been implementation specific. I'm not sure we even have any tests for that sort of behavior either. So, not anything that we would have thought about describing as a breaking change.)

[alexanderchr]

@markerikson I appreciate that, at first I took it as if you were not even considering PRs. Fully understand that this is not your top priority.

I think that unless you have a full formal specification of a library there will always be lots of common sense behavior that is nowhere documented but cannot be changed without causing a breaking change. I think that not swallowing errors in functions passed to the library is usually such behavior, but obviously understand that it's impossible to keep track of it all (and also a question of personal judgement).

[hipstersmoothie]

Any updates on this? This is blocking our upgrade path.

Switching it useSelector isn't really an option for us at this point either

[markerikson]

@hipstersmoothie : no updates . Per above, all my efforts in the last year+ have gone to upgrading the Redux packages (RTK 2.0, etc), and reworking the "Redux Essentials" tutorial.

I'm not ruling out future changes to connect completely, but working on it is not a priority any time in the near future.

If you or someone else would like to submit a PR, I'm open to looking at it.

What specifically about the connect error handling behavior is causing you problems at this time?

(Also, is there a specific reason why you can't migrate to useSelector in your codebase?)

[srubin]

(I work with @hipstersmoothie so I'll answer!)

We haven't actually been able to confirm that this bug is happening to us in 8.1.3. We're seeing errors getting bubbled up to our react error boundaries as expected, just like in react-redux 7.x. The codesandbox links in the original post seem to be dead now, so I can't test the original repro case. I'm wondering if there was some specific context around when it failed that we're not capturing in our synthetic tests. So, hopefully this is a false alarm on our end!

(Also, is there a specific reason why you can't migrate to useSelector in your codebase?)

Mostly the risks of large-scale refactoring jobs in complex codebases, i.e., ensuring that nothing breaks along the way, and that performance is not regressing. We have some fun custom wrappers for connect, so we'd need to add hook versions of those changes, as well. So, it's not that we can't do it, it's just that it'd be a nontrivial project that needs to be weighed against all of our other priorities.

[markerikson]

Yeah, I definitely understand the effort to do major migrations :)

Out of curiosity, what sort of "custom connect wrappers" do you have? What do those look like?

[srubin]

Out of curiosity, what sort of "custom connect wrappers" do you have? What do those look like?

One is related to error-tracking (we want errors thrown in mapStateToProps to be tracked to Sentry with specific additional metadata).

The more interesting one modifies the behavior of mapStateToProps to preserve reference equality for values that are deeply equal to their previous results. While this can be addressed with selectors/cached selectors/the mstp factory pattern, doing this everywhere that connect is used helps us avoid an entire class of unnecessary re-renders and simplifies code. Our data indicates that perf is as good or better than without this wrapper.