Should detect whole state spreading

[theKashey]

With a given code

const mapState = ({ page, direction, ...state }) => ({
  page,
  direction,
  isLoading: isLoading(state)
})

memoize state should not react to any state change, as it would.

Posible solutions:

1. Currently, memoize react on key get, but it could react on key read. This will require all objects to be wrapped by something with valueOf/toString. They key will be added to tracking if someone "read" it, or "return" it.
   Could affect performance. Unstable.

2. Track such situations and encourage a user to change the code

• cast function to string, and search for spread operator (unstable)
• detect then all keys of state used, and inform user of a potential problem. Easy to do, could be enabled by default in dev mode.

[faceyspacey]

warning in the console is really less than ideal. it basically is an issue similar to forgetting to do immutability, but maybe worse without the console warning.

i hope #1 is stable and performant.

[theKashey]

or, error in console. In our test environment any error colours everything in red.
So it will be as useful as propTypes, and they are useful.

[theKashey]

PS: Please bump the library version, 1.2.0 could be much more performant than 1.1.6 due to new API from proxyequal and 1.2.1 works on IE11/Android.

[faceyspacey]

i put it to 1.2.1--is that ok?

ps. i also decided to add a 2nd arg for mapDispatchToProps based on our discussion. It doesn't sound like we have a strong enough argument in favor of pursuing that route yet, so it's better not to get people focused on the wrong thing. In the current implementation, it just binds an action creators object. I haven't been using this arg or mapDispatchToProps nor mapMergeProps for over a year now, since I only use redux-first-router's <Link /> component, which performs the dispatch. Correct me if I'm wrong, but mapDispatch and mapMergeProps doesn't offer any performance gains (in fact it may be worse)--it's just so in your component code you have less arguments to pass (sometimes none)? I remember I used to juggle these additional arguments to connect in order to make component code as simple as possible. If there are no perf gains (since the component must re-render anyway with the new action creator functions), this seems just like an unnecessary over-optimization to reduce a few characters and remember less arguments to pass (perhaps the only benefit is you can pass these functions down the component tree without having to pass props they will need as well [or pre-bind them in components]). What do you think? If so, then perhaps a modern react-redux API could just be 2 simple arguments. Here's the final implementation /w 2 args:

https://codesandbox.io/s/64mzx7vjmz?module=%2Fsrc%2Fconnect%2Findex.js

[theKashey]

Correct me if I'm wrong, but mapDispatch and mapMergeProps doesn't offer any performance gains (in fact it may be worse)

mapDispatch will be executed once, to get the names for props, and absolutely does not affect performance. Thats why you can always return "a new function" from mapDispatch and not forcing component to update.

mapMergeProps is rarely used, then you have to displatch something with props you just mapped. I personaly have 1-2 examples of this.

"performance" here is simple - you have to re-render component only if first part(mapStateToProps) got updated, rest - you dont care.

[faceyspacey]

Typically, a new doSomething reference will trigger an update every time props changes:

const mapDispatch = (dispatch, props) => {
     const doSomething = (arg) => dispatch({ type: 'FOO', payload: { arg, bar: props.bar } })
     return { doSomething }
}

...unless they execute mapDispatch itself on every click or tap. And then call doSomething with the latest props? And, like you said, they get the names in the beginning once. Is that how it works?

something like the following:

props.doSomething = (...args) => mapDispatch(dispatch, this.props).doSomething(...args)
<button onClick={() => props.doSomething(123)}>

Is that what you mean by "get the names"? It simply needs to know the names to make the above assignment to props.doSomething.

If this is true, then the performance is very good for mapDispatch and far better than passing props to components to be used as arguments to handlers.

[theKashey]

Take a look here - https://github.com/reactjs/react-redux/blob/master/src/connect/selectorFactory.js#L74 - there is 3 different types of change

    if (propsChanged && stateChanged) return handleNewPropsAndNewState()
    if (propsChanged) return handleNewProps()
    if (stateChanged) return handleNewState()

In the same time it tracks "source" of the change - if (mapDispatchToProps.dependsOnOwnProps)

But!
mapStateToProps depends on state, and may depend on props
mapDispatch, may depend on props, (but usually not)
mergeProps depends on these two "sources" change, and, will be executed each time, to merge even unchanged results :(

I might be wrong, but look like if your dispatches depend on props - any memoization is fucked, as long it may always produce new functions.

Could be fixed

1. Wrap dispatches by "proxies" (not the Proxies)

const wrapDispatchers = (mapDispatchToProps, dispatch) => {
  const factory = normalize(mapDispatchToProps);// https://github.com/reactjs/react-redux/blob/master/src/connect/mapDispatchToProps.js
  const dispatchs = factory();
  const names = Object.keys(dispatchs);
 
 // create forward proxy
  return names.reduce( (map, key) => {
    // return a function, which on-call will execute factory once more and call the real action.
    map.key = (...args) => dispatch(factory(dispatch, ownProps)[key](...args)
    return map;
  }, {}); 
}

2. Still, have to use merge props. It should be called if ownProps or result from mapStateToProps changes.

But - one could remove "areStatesEqual" at all.

[theKashey]

Just found that internals of memoization are discoverable in React tools

[faceyspacey]

the only issue I see is this: the child component will re-render no matter what if the props changes. so it does not matter that we kept the reference returned from mapDispatchToProps equal. It perhaps only matters in terms of the implementation in this library, where quick shallow comparisons are iterated through.

I'm not seeing how this is preventing re-renderings in the immediate child, perhaps in the child's children it's nice they receive the same function reference.

[theKashey]

Maybe I've lost a thread, but you have shouldComponentUpdate for it?

[faceyspacey]

...i mean we can do anything for our own libs. im just trying to figure out what the exact thinking of react-redux is.

it seems the "optimization" is simply that if ownProps is not used, it won't call mapDispatchToProps every time,:

https://github.com/reactjs/react-redux/blob/master/src/connect/selectorFactory.js#L57
https://github.com/reactjs/react-redux/blob/master/src/connect/wrapMapToProps.js#L47

note, I think these lines are unnecessary since the logic is handled above:
https://github.com/reactjs/react-redux/blob/master/src/connect/wrapMapToProps.js#L41-L43

---

So this means, when there are new props, there will be new handlers. Plain and simple. There is no optimization for that. But it could be possible if the parent updated, this:

https://github.com/reactjs/react-redux/blob/master/src/connect/selectorFactory.js#L29

and did this: shouldComponentUpdate() => false. Then the existing handler reference would have access to the new props, while the child component did not re-render.

They are not doing this though :)

Well, they/we couldn't do it generally anyway, as the child component needs those new props for its own purposes.

The react-redux library ultimately is doing very little with a whole lot of code. This is like the 4th time in several years that I've spent several hours analyzing it. It's not worth it. This can and should be far simpler. If you're memoize-state library can work in my demo like that, we have a very powerful solution. Nobody needs mapDispatchToProps in function form--BECAUSE THERE ARE NO PERFORMANCE OPTIMIZATION AS A RESULT OF IT! There's only worse performance from using a function here instead of an object of action creators. Even if it tries to maintain the same function references, it wouldn't matter because the child component will need to re-render anyway with the new props. That's why my demo + memoize-state may in fact be a complete solution right here, right now. No more react-redux. Provided, the new way of using the new context API to "broadcast" subscriptions is in fact high quality (and at least about the same speed as the old implementation).

[faceyspacey]

that's not to say, memoizing action creators is a good idea. lets forget about that altogether. ..im just trying to figure out what we really need to be a high quality abstraction.

my question simply was this:

- can we prevent re-renders by binding some props/arguments in connect vs. passing all as args in component code?

I don't think there is any performance difference (in the general case). The wrapped component must re-render in both cases.

THEREFORE, who cares about this signature:
connect(mapState: Function, mapDispatch: Function)

all that matters is this one:
connect(mapState: Function, mapDispatch: Object)

just like in the demo. if the performance gains hypothesis I just made can be disproved, we need to know. But if not, we can forget about mapDispatch in function form, and therefore we can forget about mapMergeProps. Cuz that's only needed if you use mapDispatch in function form.

Then we have a simple formula for react-redux-simple.

[theKashey]

Keep in mind - there is 2 things to solve

1. Let the old code work. To much code was already written
2. Evolve.

This means - one could try to make a better react-redux(just move your current experiment to the real repo), and also provide react-redux-compact to let the "old" users use the newish redux adapter.

This also means that there must be some "migration stories" about how to do "something" with old react-redux, and how to do the same with a new. And why the new is better.
And it will take some time to make it better, as long there are a lot of stories we don't cover, we don't discuss yet.

[faceyspacey]

i dont care about react-redux or the old ways. im building my own Framework :)

the old ways are all garbage. if we followed the mainstream, we'd be composing everything until our apps collapse instead of developing far more fluidly with Redux-First Router. You're a user of that lib right?

Either way, the kung fu of Reactlandia is more "authentic" than what's main stream :) and there's a lot of new things coming. I'm just trying to collate all the optimizations of react-redux so none are missing. Basically, it has no crazy optimizations, just support for stupid use cases that make for an unnecessarily complex API, and which can be solved other ways.

Respond Framework will have components like this FYI:

const MyComponent => (props, state, actions) =>
aka
const MyComponent => (props, state, events) =>

no HoCs necessary. lazily memoized "UI database" based on your reducers + all selectors-- this is what's in the state arg above. ..events is the mapDispatchToProps arg in object form, but containing all action creators of the whole app. forget all the hoc shit. ..state will use characteristics of what you're doing with memoize-state + Immer.

no more language about "action creators." same redux functionality. the overall concept: "what if we built React with a global state store in mind from the start?"

ps. RFR is about to have a big release that has been 6+ months in the making. It will be the foundation of Respond Framework.

"Don't just React, Respond" :)

Stage 2 is making Remixx the Redux-compatible replacement that provides the above simplified component api, + some stuff of MobX, plus "redux modules" etc. That's why I'm doing all this research. It may be something that makes sense for us to collaborate on. You have a lot of experience now with the proxy-based "access" strategies--that will be the next area I move into. Currently, I'm no expert in that area, but will need to be to make the above happen.