reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.37k stars 3.37k forks source link

Weird delay when using mapDispatchToProps shorthand notation #642

Closed davegri closed 7 years ago

davegri commented 7 years ago

The following code works fine:

const mapDispatchToProps = dispatch => ({
  retrieveAnnouncements: () => dispatch(retrieveAnnouncements()),
  selectNextAnnouncement: () => dispatch(selectNextAnnouncement()),
  selectPreviousAnnouncement: () => dispatch(selectPreviousAnnouncement())
})

but switching it out with this causes an 8 second delay between mapStateToProps and the associated component.

const mapDispatchToProps = {
  retrieveAnnouncements,
  selectNextAnnouncement,
  selectPreviousAnnouncement
}

Chrome console reports

[Violation] Handler took 9291ms of runtime (150ms allowed)

on:

"use strict";
/* harmony export (immutable) */ __webpack_exports__["a"] = bindActionCreators;
function bindActionCreator(actionCreator, dispatch) {
  return function () {
    return dispatch(actionCreator.apply(undefined, arguments));
  };
}

any ideas?

markerikson commented 7 years ago

That... seems extremely odd. Do you have a repro of the issue, or any kind of snapshot of the perf behavior?

As you can see, bindActionCreators is itself extremely trivial, and that shouldn't be causing any kind of problem.

davegri commented 7 years ago

Yeah this is a super weird issue, no idea what exactly is causing this.. I'm not sure really how to read a CPU profile but here is the file

CPU-20170306T114944.txt

I'm using redux-actions to create action creators like so:

import { createAction } from 'redux-actions'
import ACTIONS from 'constants/actionsConstants'

export const selectNextAnnouncement = createAction(ACTIONS.ANNOUNCEMENTS.NEXT)
export const selectPreviousAnnouncement = createAction(ACTIONS.ANNOUNCEMENTS.PREVIOUS)

and importing inside the container

import { selectNextAnnouncement, selectPreviousAnnouncement } from 'redux/actions/announcements'

const mapStateToProps = (state) => {
  const { announcements, activeAnnouncementIndex, isLoading, totalAnnouncements } = state.announcements
  return {
    activeAnnouncementIndex,
    totalAnnouncements,
    announcements,
    isLoading
  }
}

const mapDispatchToProps = {
  retrieveAnnouncements,
  selectNextAnnouncement,
  selectPreviousAnnouncement
}

export default connect(mapStateToProps, mapDispatchToProps)(AnnouncementsContainer)

If I put a console log in both the mapStateToProps and render function of AnnouncementsContainer the mapState console log happens right away but the render function only happens 8 seconds later.

timdorr commented 7 years ago

Based on that CPU profile (really helpful, btw!), the only thing I can see that's related is this warning. That's the only place we call stringify, which is the last of the big block of CPU time on the flamegraph:

screen shot 2017-03-06 at 12 15 43 pm

The only reason I think it might not be that is because the parent functions are anonymous and the parent to our stringify usage is a named function (wrapWithConnect). If you click on the block in the flamegraph, it will send you to that code in the Sources tab. If you have sourcemaps turned on, it will show you the exact module/file where that code lives. You can hover over the tab to get the full location if it doesn't jump out to you who actually owns that code.

Go ahead and do that so we can narrow things down.

davegri commented 7 years ago

Clicking on the anonymous before the stringify sends me to

 return function (action) {
        state = getState();

        result = tracker.detectMutations();
        // Track before potentially not meeting the invariant
        tracker = track(state);

        (0, _invariant2['default'])(!result.wasMutated, BETWEEN_DISPATCHES_MESSAGE, (result.path || []).join('.'));

        var dispatchedAction = next(action);
        state = getState();

        result = tracker.detectMutations();
        // Track before potentially not meeting the invariant
        tracker = track(state);

        (0, _invariant2['default'])(!result.wasMutated, INSIDE_DISPATCH_MESSAGE, (result.path || []).join('.'), (0, _jsonStringifySafe2['default'])(action));

        return dispatchedAction;
      };

which looks like the state mutations detection middleware redux-immutable-state-invariant

Any ideas how this can be related? what should I look at next?

timdorr commented 7 years ago

Well, it looks like you're causing an invariant to trip inside that library and it's trying to print the full contents of the action, which must include a sizable payload (hence the 8 second delay).

Regardless, it looks like it might be an overly eager check by that middleware for immutable mutations. I didn't go into too much detail on it, but that's the general idea. It looks like a development aid, so removing it should be harmless. You're probably better off getting rid of it.

markerikson commented 7 years ago

FWIW, there's several other options for similar checks as well: https://github.com/markerikson/redux-ecosystem-links/blob/master/devtools.md#linting .

davegri commented 7 years ago

But how does changing from shorthand to the full mapDispatchToProps fix this? I'm still confused Also there is no invariant warning printed at the end of the 8 seconds, and the payload is tiny..

timdorr commented 7 years ago

My best guess is it is accessing some property of the action creator somewhere that is tripping the that middleware. By wrapping them in functions, you're delaying that access until later, when it might not trip the middleware. I'm not an expert in that library, so I don't know for sure. I would just remove it.