Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connect swallows errors in mapStateToProps (new bug in 8.0.0) #1942

Open
1 task done
grumd opened this issue Aug 17, 2022 · 17 comments
Open
1 task done

connect swallows errors in mapStateToProps (new bug in 8.0.0) #1942

grumd opened this issue Aug 17, 2022 · 17 comments

Comments

@grumd
Copy link

grumd commented Aug 17, 2022

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
  };
};
  1. Dispatch an action that changes the state so that it causes the error above
dispatch(updateFoo(undefined));
  1. 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
image

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
Copy link
Author

grumd commented Aug 17, 2022

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.

@grumd grumd changed the title connect swallows errors in mapStateToProps connect swallows errors in mapStateToProps (new bug in 8.0.0) Aug 17, 2022
@markerikson
Copy link
Contributor

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
Copy link
Author

grumd commented Aug 17, 2022

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
Copy link
Contributor

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
Copy link
Author

grumd commented Aug 17, 2022

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
Copy link

itsjohncs commented Sep 5, 2022

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
Copy link

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:

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
Copy link
Contributor

@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
Copy link

alexanderchr commented Sep 21, 2023

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
Copy link

alexanderchr commented Sep 21, 2023

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

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
Copy link
Contributor

markerikson commented Sep 21, 2023

@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
Copy link

@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
Copy link

hipstersmoothie commented Jul 30, 2024

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
Copy link
Contributor

@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
Copy link

srubin commented Jul 30, 2024

(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
Copy link
Contributor

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
Copy link

srubin commented Jul 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants