Error handling in connectAdvanced can swallow errors

[markerikson]

Reported in Reactiflux chat by user "Bahamut", who was not in a position to file a report himself.

Description:

There was a function being defined inside of a mapDispatch function, like:

function mapDispatch(dispatch) {
    return {
        someFunction: () => dispatch({type : "SOME_ACTION", string : `some template literal`})
    };
}

There was an error in the template literal, and that threw an error when the mapDispatch function was checked by connectAdvanced() at connectAdvanced.js#L15-25.

However, the error was swallowed by the try/catch block there, and no visible error was reported in the console.

So, the important issue here appears to be that we have a try/catch that swallows errors, and doesn't report them in any way.

Would it be a good idea to use the warning() util to log errors here?

[hoschi]

We found the same problem with when our selector function had an error (accessing prop 'x' from undefined) which was swallowed.

[rick-li]

Same here, it swallows all the run time errors in render, it can waste days debugging it.... :(

[markerikson]

@rick-li : I wouldn't expect this to swallow errors from rendering, just errors that occur inside of a mapState or mapDispatch function.

[mjniday]

I just had this happen with mapState where errors were not surfaced. I was trying to access a property on something inside its ownProps which was temporarily undefined. Less certain still about this part, but the browser kept crashing when trying to render that component. Fixing the error in the mapState function resolved that issue for me.

Something like...

const mapState = (state, ownProps) => {
  const itemId = ownProps.collection.first().itemId // ownProps.collection.first() was initially undefined
  return {
    itemId
  }
}

Seems possible that the error handling here was blowing up the call stack which resulted in the browser dying.

[jimbolla]

The error should be rethrown in render() here

[AlexTwine]

This needs to be fix. I spent a good amount of time trying to figure out why my debugger wasn't getting hit in the mapState function. The error should show up in the console.

[dreadwail]

I was going to look into this but in all my attempts to reproduce the issue I cannot due to the reasons @jimbolla outlined above. The errors appear to be getting correctly re-thrown inside of the Connect component render().

Am I missing something?

My mechanism for testing was to simply throw an error inside of a string template within mapDispatchToProps or mapStateToProps.

import Counter from './Counter';
import { incrementAction } from '../actions';
import { connect } from 'react-redux';

const blowup = () => {
  throw new Error('BOOM');
};

const mapStateToProps = (state) => ({
  text: `test ${blowup()}`,
  count: state.count
});

const mapDispatchToProps = (dispatch) => ({
  onClick: () => dispatch(incrementAction())
});

export default connect(mapStateToProps, mapDispatchToProps)(Counter);

The correct stuff shows up in a browser because of the re-thrown error:

[janwirth]

Did anyone find a workaround for this?

[zengyang2014]

Hi @markerikson Why not throw error directly once catch errors in selectors?

function makeUpdater(sourceSelector, store) {
  return function updater(props, prevState) {
    try {
      const nextProps = sourceSelector(store.getState(), props)
      if (nextProps !== prevState.props || prevState.error) {
        return {
          shouldComponentUpdate: true,
          props: nextProps,
          error: null,
        }
      }
      return {
        shouldComponentUpdate: false,
      }
    } catch (error) {
      threw error
    }
  }
}

[sle-c]

for some reasons try catch block to re-throw error does not work for me either

const todoSelector = (state) => {
  return state.todos;
};

const presentTodos = createSelector(
  [todoSelector],
  (todos) => {
    throw new Error("boom");
  }
);
const mapStateToProps = (state) => {
  try {
      return {
        todos: presentTodos(state),
      };
  } catch (e) {
    throw e; // this is still being swallowed by connect if the errors happen inside the selector
  }
}

So instead, i have to to this to bubble up the errors

const todoSelector = (state) => {
  return state.todos;
};

const presentTodos = createSelector(
  [todoSelector],
  (todos) => {
    throw new Error("boom");
  }
);
const mapStateToProps = (state) => {
  try {
      return {
        todos: presentTodos(state),
      };
  } catch (e) {
    console.error(e); // now this works
  }
}

[markerikson]

I'm not quite ready to close this yet, but we really need to see a reproduction of this issue to do anything with it. If anyone is seeing this problem of connect() swallowing errors without re-throwing them, please put together a minimal repro of the problem and link that here.

[Nantris]

@markerikson is this believed to be fixed at this point? It seems not to be, or at least - I have no other theory for why our errors are getting swallowed inside of our mapStateToProps.

I've spent an hour trying to pinpoint the exact cause for us, and this issue is my only lead.

Specifically, this component uses forwardRef - so I wonder if that could be related? Version 8.0.4. I discovered this when I accidentally typed 'some text'.contains('') instead of 'some text'.includes('') and everything broke, but no error appears.

If I call the function from anywhere else, or manually from console - the errors appear as expected.

The only other thing that's really even remotely of interest that I can see is that the function that's called is memoized.

I've seen errors properly logged from mapStateToProps plenty of times - but it just won't work here. Coincidentally, this is one of only four components we have with forwardRef: true.

[markerikson]

@slapbox : we're multiple major versions and internal rearchitectures past when this issue was closed :) Could you open up a separate new issue so we can keep track of your question on its own, and if at all possible include either a CodeSandbox showing the problem or a Replay ( https://replay.io/record-bugs ) of this happening?

[Nantris]

Hey @markerikson thanks for the reply! I know I'm super late to the party.

Unfortunately I won't have the time to do anything besides basically copy paste that comment into a new issue (and let me know if I should) - as the solution for us was simply to move that function into render (where it should have been anyway.)

[markerikson]

Realistically, I'm not going to have time to look at this any time soon. But I definitely won't ever remember to look at it if there's no new issue filed :)