{"action":"created","issue":{"url":"https://api.github.com/repos/reactjs/react-redux/issues/416","repository_url":"https://api.github.com/repos/reactjs/react-redux","labels_url":"https://api.github.com/repos/reactjs/react-redux/issues/416/labels{/name}","comments_url":"https://api.github.com/repos/reactjs/react-redux/issues/416/comments","events_url":"https://api.github.com/repos/reactjs/react-redux/issues/416/events","html_url":"https://github.com/reactjs/react-redux/pull/416","id":162071270,"number":416,"title":"Rewrite connect() for better performance and extensibility","user":{"login":"jimbolla","id":1851364,"avatar_url":"https://avatars.githubusercontent.com/u/1851364?v=3","gravatar_id":"","url":"https://api.github.com/users/jimbolla","html_url":"https://github.com/jimbolla","followers_url":"https://api.github.com/users/jimbolla/followers","following_url":"https://api.github.com/users/jimbolla/following{/other_user}","gists_url":"https://api.github.com/users/jimbolla/gists{/gist_id}","starred_url":"https://api.github.com/users/jimbolla/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jimbolla/subscriptions","organizations_url":"https://api.github.com/users/jimbolla/orgs","repos_url":"https://api.github.com/users/jimbolla/repos","events_url":"https://api.github.com/users/jimbolla/events{/privacy}","received_events_url":"https://api.github.com/users/jimbolla/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":26,"created_at":"2016-06-24T03:53:41Z","updated_at":"2016-07-05T18:14:35Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/reactjs/react-redux/pulls/416","html_url":"https://github.com/reactjs/react-redux/pull/416","diff_url":"https://github.com/reactjs/react-redux/pull/416.diff","patch_url":"https://github.com/reactjs/react-redux/pull/416.patch"},"body":"# TL;DR\r\n\r\nRewrote `connect`, same basic API plus advanced options/API, all tests pass, roughly 8x faster, more modular/extensible design\r\n\r\n# Overview\r\n\r\nI rewrote `connect` into modular pieces because I wanted to be able to extend with custom behavior in my own projects. Now connect is a facade around `connectAdvanced`, but passing it a compatible `selectorFactory` function.\r\n\r\nI also was able to greatly improve performance by changing the \"sideways\" subscription pattern into\r\na heirarchial pattern to work  with React's natural flow of updates; component instances lower in the tree always get updated after those above them, avoiding unnecessary re-renders.\r\n\r\n# Design/Architecture\r\n\r\nI split the original `connect` into many functions+files to compartmentalize concepts for better readability and extensibility:\r\n\r\n- components/\r\n - `connect.js`: composes the other functions into a backwards-compatible API, by creating a selectorFactory and options object to pass to `connectAdvanced`.\r\n - `connectAdvanced.js`: the HOC that connects to the store and determines when to re-render\r\n- selectors/\r\n - `createFactoryAwareSelector.js`: a selector helper used by `mapDispatch.js` and `mapState.js` to translate a function value for `mapStateToProps` or `mapDispatchToProps` into a selector function that performs memoiztion and detects if the first run returns another function, indicating a factory\r\n - `getFinalProps.js`: combines and memoizes the results of selector for `mapStateToProps`, the selector for `mapDispatchToProps`, and `mergeProps` into a connectAdvanced-compatible selector\r\n - `mapDispatch.js`: used to create a selector from the `mapDispatchToProps` parameter, to be passed to `getFinalProps.js`. Detects whether `mapDispatchToProps` is missing, an object, or a function\r\n - `mapState.js`: used to create a selector from the mapStateToProps parameter, to be passed to `getFinalProps.js`. Detects whether `mapStateToProps` is missing or a function\r\n - `mergeProps.js`: has the default implementation of the `mergeProps` parameter\r\n- utils/\r\n - `memoizeProps.js`: wraps incoming and outgoing props in shallowEquals memoization to optimize equality checks in `connectAdvanced.js` and `createFactoryAwareSelector.js`\r\n - `Subscription.js`: encapsulates the hierachial subscription concept. used by `connectAdvanced.js` to pass a parent's store Subscription to its children via context\r\n - `verifyPlainObject.js`: used to show a warning if `mapStateToProps`, `mapDispatchToProps`, or `mergeProps` returns something other than a plain object\r\n\r\n## function graph\r\n![graph](https://cloud.githubusercontent.com/assets/1851364/16338107/81381130-39e8-11e6-9f0d-118b19bb2545.png)\r\n ```\r\ndigraph functions {\r\n  wrapWithVerify -> verifyPlainObject\r\n  selectorFactory -> { addStateAndDispatchSelectors wrapWithVerify createFinalPropsSelector }\r\n  buildOptions -> { defaultMapDispatchFactories defaultMapStateFactories defaultMergeProps }\r\n  connect -> { buildOptions selectorFactory connectAdvanced }\r\n\r\n  connectAdvanced -> { memoizeProps storeShape Subscription }\r\n\r\n  createImpureFactoryAwareSelector -> createMapOrMapFactoryProxy\r\n  createPureFactoryAwareSelector -> { createMapOrMapFactoryProxy memoizeProps }\r\n  createFactoryAwareSelector -> { createImpureFactoryAwareSelector createPureFactoryAwareSelector }\r\n\r\n  createFinalPropsSelector -> { createImpureFinalPropsSelector createPureFinalPropsSelector }\r\n\r\n  whenMapDispatchIsFunction -> createFactoryAwareSelector\r\n  defaultMapDispatchFactories -> { whenMapDispatchIsMissing whenMapDispatchIsFunction whenMapDispatchIsObject }\r\n\r\n  whenMapStateIsFunction -> createFactoryAwareSelector\r\n  defaultMapStateFactories -> { whenMapStateIsMissing whenMapStateIsFunction }\r\n\r\n  memoizeProps -> shallowEqual\r\n}\r\n```\r\n\r\nThe modular structure of all the functions in `selectors/` should allow greater reuse for anyone that wants to create their own `connect` variant. For example, one can create a variant that handles when `mapStateToProps` is an object:\r\n\r\n### customConnect.js:\r\n```javascript\r\nimport connect from 'react-redux/lib/connect'\r\nimport { getDefaultMapStateFactories } from '../selectors/mapState'\r\n\r\nfunction whenStateIsObject({ mapStateToProps }) {\r\n  if (mapStateToProps && typeof mapStateToProps === 'object') {\r\n    const selector = createStructuredSelector(mapStateToProps)\r\n    return (state, props) => selector(state, props)\r\n  }\r\n}\r\n\r\nexport default function customConnect(mapStateToProps, mapDispatchToProps, mergeProps, options) {\r\n  return connect(mapStateToProps, mapDispatchToProps, mergeProps, {\r\n    ...options,\r\n    mapStateFactories: getDefaultMapStateFactories().concat(whenStateIsObject)\r\n  })\r\n}\r\n\r\n```\r\n### ExampleComponent.js\r\n```javascript\r\n// ...\r\nexport default customConnect({\r\n  thing: thingSelector,\r\n  thong: thongSelector\r\n})(ExampleComponent)\r\n```\r\n\r\nAnd for scenarios where connect's three-function API is too constrictive, one can directly call, or build a wrapper around, `connectAdvanced` where they have full control over turning `(state, props, dispatch)` into a props object.\r\n\r\n\r\n# Performance\r\n\r\nI'm using a [modified version of react-redux-perf](https://github.com/jimbolla/react-redux-perf) to performance test+profile the changes. It's configured to try to fire up to 200 actions per second (but becomes CPU bound), with 301 connected components. There are 2 scenarios being tested:\r\n- NB: a parent component with 300 child components, with no other React components between them.\r\n- WB: the same setup as NB but there's a \"Blocker\" React component between the parent and children that always returns false for `shouldComponentUpdate`.\r\n\r\nI measured the milliseconds needed to render a frame using the [stats.js](https://github.com/mrdoob/stats.js/) used by react-redux-perf:\r\n\r\nMS: avg (min-max) | current NB | rewrite NB | current WB | rewrite WB\r\n----------------- | ---------- | ---------- | ---------- | ----------\r\nChrome | 170 (159-223) | 20 (17-55) | 170 (167-231) | 17 (15-59)\r\nFirefox | 370 (331-567) | 20 (16-51) | 430 (371-606) | 19 (15-60)\r\nIE11 | 270 (127-301) | 40 (36-128) | 300 (129-323) | 33 (30-124)\r\nEdge | 240 (220-371) | 37 (32-102) | 260 (97-318) | 28 (24-100)\r\n\r\nOn the conservitive end, the rewrite is about 8x faster under these circumstances, with Firefox even doubling that improvement. Much of the perf gains are attributed to avoiding calls to `setState()` after a store update unless a re-render is necessary.\r\n\r\nIn order to make that work with nested connected components, store subscriptions were changed from \"sideways\" to top-down; parent components always update before their child components. Connected components detected whether they are nested by looking for an object of type `Subscription` in the React `context` with the key `storeSubscription`. This allows `Subscription` objects build into a composite pattern.\r\n\r\nAfter that I've used Chrome and Firefox's profilers to watch for functions that could be optimized. At this point, the most expensive method is `shallowEqual`, accounting for 4% and 1.5% CPU in Chrome and Firefox, respectively. \r\n\r\n# connectAdvanced(selectorFactory, options) API\r\n\r\nIn addition to the changed related to performance, the other key change is an additional API for `connectAdvanced()`. `connectAdvanced` is now the base for `connect` but is less opinionated about how to combine `state`, `props`, and `dispatch`. It makes no assumptions about defaults or intermediate memoization of results, and leaves those concerns up to the caller. It does memoize the inbound and outbound props objects. A full signature for `connectAdvanced` with its `selectorFactory` would look like:\r\n\r\n```javascript\r\nexport default connectAdvanced(\r\n  // selectorFactory receives dispatch and lots of metadata as named parameters. Most won't be\r\n  // useful to a direct caller, but could be useful to a library author wrapping connectAdvanced\r\n  function selectorFactory({ dispatch, displayName, methodName, pure, storeKey, withRef, WrappedComponent, ...extras }) {\r\n\r\n    // dispatch is also available as the 3rd parameter passed to the selector func. This\r\n    // is useful when using a memoization library such as Reselect to memoize the binding\r\n    // of your actionCreators to dispatch and props/state values\r\n    return function(nextState, nextProps, thisIsAlsoDispatch) {\r\n\r\n      return {\r\n        someProp: //...\r\n        anotherProp: //...\r\n      }\r\n    }\r\n  },\r\n  // options with their defaults:\r\n  {\r\n    dependsOnState: true,\r\n    getDisplayName: name => `ConnectAdvanced(${name})`,\r\n    methodName: 'connectAdvanced',\r\n    pure: true,\r\n    recomputationsProp: undefined,\r\n    storeKey: 'store',\r\n    withRef: false,\r\n    // you can also add extra options that will be passed through to your selectorFactory:\r\n    custom1: 'hey',\r\n    fizzbuzz: fizzbuzzFunc\r\n  }\r\n)(MyComponent)\r\n\r\n```\r\n\r\nA simple usage may look like:\r\n```javascript\r\nexport default connectAdvanced(({ dispatch }) => {\r\n  const bound = bindActionCreators(actionCreators, dispatch)\r\n  return (store, props) => ({\r\n    thing: state.things[props.thingId],\r\n    saveThing: bound.saveThing\r\n  })\r\n})(MyComponent)\r\n\r\n```\r\n\r\nAn example using `reselect` to create a bound actionCreator with a prop partially bound:\r\n```javascript\r\nexport default connectAdvanced(({ dispatch }) => {\r\n  return createStructuredSelector({\r\n    thing: (state, props) => state.things[props.thingId],\r\n    saveThing: createSelector(\r\n      (_, props) => props.thingId,\r\n      (thingId) => inputs => dispatch(actionCreators.saveThing(thingId, inputs))\r\n    )\r\n  })\r\n})(MyComponent)\r\n```\r\n\r\nNote these are meant as examples and not necessarily \"best practices.\"\r\n\r\n# Pros/cons\r\n\r\nI understand there is great risk to accepting such drastic changes, that would have to be justified with significant benefits. I'll reiterate the two main benefits I believe these changes offer:\r\n\r\n1. Performance: There's potentially huge perf gains in situations where the number of connected components is high, stemming from conceptual changes to subscriptions so they go with the natural flow of events in React vs across them, as well as method profiling+optimizing using Chrome/FF.\r\n2. Extensibility/Maintainability: By splitting the responibilities of connect into many smaller functions, it should be easier both for react-redux contributors to work with the codebase and end users to extend its functionality though the additional APIs. If users can add their desired features in their own projects, that will reduce the number of feature requests to the core project.\r\n\r\nDespite passing all the automated tests as well as week of manual testing, there is risk of impacting users dependant on implicit behavior, or that performance is worse in some unexpected circumstances. To minimize risk of impacting end users and downstream library authors, I think it would be wise to pull these changes into a \"vNext\" branch and first release an alpha package. This would give early adopters a chance to test it and provide feedback\r\n\r\n# Thanks\r\n\r\nI'd like to thank the other github users who have so far offered feedback on these changes in #407, especially @markerikson who has gone above and beyond.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/reactjs/react-redux/issues/comments/230558143","html_url":"https://github.com/reactjs/react-redux/pull/416#issuecomment-230558143","issue_url":"https://api.github.com/repos/reactjs/react-redux/issues/416","id":230558143,"user":{"login":"jimbolla","id":1851364,"avatar_url":"https://avatars.githubusercontent.com/u/1851364?v=3","gravatar_id":"","url":"https://api.github.com/users/jimbolla","html_url":"https://github.com/jimbolla","followers_url":"https://api.github.com/users/jimbolla/followers","following_url":"https://api.github.com/users/jimbolla/following{/other_user}","gists_url":"https://api.github.com/users/jimbolla/gists{/gist_id}","starred_url":"https://api.github.com/users/jimbolla/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jimbolla/subscriptions","organizations_url":"https://api.github.com/users/jimbolla/orgs","repos_url":"https://api.github.com/users/jimbolla/repos","events_url":"https://api.github.com/users/jimbolla/events{/privacy}","received_events_url":"https://api.github.com/users/jimbolla/received_events","type":"User","site_admin":false},"created_at":"2016-07-05T18:14:35Z","updated_at":"2016-07-05T18:14:35Z","body":"I think tagging it with `@rewrite` or similar is a good idea. I don't intend to make any other changes right now besides add more comments unless anyone has any feedback on things that could be changed."}}
{"actor":{"display_login":"jimbolla"}}