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.