make work as general react-redux memoizer

[faceyspacey]

Hey Anton, so basically for one we need it to work in the demo:

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

You mentioned that function references was the primary issue. So we need that.

Here's a primary test that must work:

static getDerivedStateFromProps(nextProps, prevState) {
        const { storeState, ...props } = nextProps
        const state = selector(storeState, props)

        console.log(prevState === state) // this must be true, when we are expecting it to be true :)

        return prevState === state ? null : state
}

Then dispatch({ type: 'BAR' }). The comparison will always be will always be false, even when nothing in state has changed. So hopefully this is just a matter of making the dispatch key/val work.

---

As far as this:

const fn = memoize(obj => ({ obj })
fn({ foo: 123 }) === fn({ foo: 123 })

Perhaps, we don't in fact need that. So let's just forget that for now.

Feel free to fork the demo, upgrade the deps, and paste a link to the working demo on the react-redux PR. I think it would be quite impressive to see all this come together in that demo. So for now, let's just think about what's actually being done in the demo, and then later I'm sure other cases will be brought to us.

[faceyspacey]

for example, this one is not working:

const mapState = ({ category, videosByCategory, videosHash }, { dispatch }) => {
  const slugs = videosByCategory[category] || []
  const videos = slugs.map(slug => videosHash[slug])
  return { videos, dispatch }
}

[faceyspacey]

I'm thinking we just need to do this:

const state = selector(storeState, props)
return isShallowEqual(state, prevState) ? null : state

Perhaps that's the perfect combination with your library. I just wasnt sure if memoize-state was not working. But if the nested objects have good guarantees on being equal object references, and we just need to do a single shallow equal on the resulting object, then I think we are on good hands.

[theKashey]

Yeah, so first of all isShallowEqual will solve some moments, even then memoize-state actually gonna to return equal states.
I am not sure yet why example you posts does not work, but there is also "ariry" related problem.
If you pass state and props, but do not access the props - memoize will think that you need props as objects and equal value to the prev state (almost always false).
This will occur if mapStateToProps does not have the second parameter at all.

The correct way to use memoizeState for "redux" is

memoize(mapStateToProps, {strictArguments: true});

It will reduce passed arguments to the real function arity.

PS: But this is not related to your problem.

So - I've got the task to solve, give me some time to play with it.

[faceyspacey]

cool.

...got it working well enough with the additional shallow equal check:

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

with this simple/sample API, we always pass both state + props, so the arity is always 2. I tried it both with and without strictArity: true and it works the same in both (i.e. I'm getting true for shallowEqual(state, prev) when I should.

Im gonna delete my last comment on the original PR about this not working. I just misunderstood that the returned object shouldn't be compared, but rather one level deep.

That said, it almost feels like it should return the exact same object reference as last time if all the key/vals are equal. That saves having to do an additional shallow equality check. I think you might already have this info before memoize returns??

[theKashey]

, it almost feels like it should return the exact same object reference as last time if all the key/vals are equal.

You know, you are actually was and is right about it. I am not sure what shallow equal did help.

[faceyspacey]

try logging state + prev in codesandbox to see. ..what I did was:

static getDerivedStateFromProps(nextProps, prevState) {
        const { storeState, ...props } = nextProps
        const state = selector(storeState, props)

        if (state.videos)  { // consistently capture the results to same selector that has `state.videos`
             window.state = state
             window.prev = prev
        }

        return shallowEqual(state, prevState) ? null : state // `null` indicates no state changes occurred
      }

and then played with the memoize function on these until I figured out what was going on. ..the nested keys are equal, but the parent object reference is not. if you keep track of which nested refs/values are equal as you operate on them, then you know when you are done if you can reuse the original object reference, and then you can just return that. ..that will make your library really shine and catch the attention of the react-redux people. Right now, those same people are realizing the value of Immer (they all really like it, and are considering using it with reducers). They're also re-thinking future react-redux APIs. I know they want to keep compatibility of course, but now is the perfect time to start thinking about new react-redux apis for the future (similar to how both the old context API + new render props context API will overlap for a little). So these people liked Immer for reducers, maybe you're immer/mobx-like approach for reselect is something that will also catch their attention. Be the one to make this painfully clear--thats why i wanted this demo to rock!

ps: i think the mapState, mapDispatch, mapMergeProps is friggin stupid. You don't really need to ever mapDispatch--it's just a convenience so your components can pass less args to handlers. In addition, mapMergeProps is a solution to the problem of having the mapDispatch arg which doesn't have access to the selected state. Now factor in there is this problem of "redux being too complicated" and lots of learners in the world. Well, give them one damn argument to merge it all, auto-memoize it, and call a fucking day. That's the thinking for the basic everyday-programmer approach. In short, react-redux v6 is done in that demo once you remove the need for calling shallowEqual. ..I also think HMR might work automatically with this simplified setup. Before they cached all the different arities/etc you could do, and perhaps HMR broke for that. If not, I had these HMR issues with React Universal Component, it's not a problem to bring back, and cleaner than before. ..Provided that the new context api is as fast as the previous techniques, nobody even needs react-redux anymore--just roll your own, and make a few tweaks as needed. The code I wrote is barely worth a damn--just the obvious way to use a new API few have used yet.

[theKashey]

So currently the goal is to understand what that is missing.
Few things should just be different, for example

 const mapStateToProps = (state, props) => {
   onAction: () => dispatch(state.something, props.something)
 };

It did not access something, and function will not be "regenerated" on those keys change. What one could do – pass not the real state and props (ok, that are already not the real), but something to access "the latest" state and props(yet another proxy). Ie, when you will execute onAction, they will do "something" with current "something" values.
But, as long keep track of the "latest" state is an easy thing, tracking the "latest" props, especially if you have more that one component, could be a problem.
Solvable or by having memoization per connected component (you can't do it with static getDerivedStateFromProps) or moving functions like this off mapStateToProps.

I am ok with simplifying things, but we have to found a way to handle simplified thing.
Could it be an API?

 const mapStateToProps = (state, props) => {
  // pass state and props to function
   onAction: (state, props) => dispatch(state.something, props.something)
 };

Nope, you could not prevent a user from accessing variables from a scope, which does not work.

Imho, splitting the function as "the old" redux do - a way to easy writing, easy testing, and easy using. But, as long you have dispatch as a prop, who will stop you from going in a "new" way?

Another way, is just to call the stored function with fake dispatch on selector creation. Or on first component mount. This will detect major keys memoize should react for, but will also require some way to inject "additional" keys to track. And may have side effects.

So many ways to go, and all of them may require something "external" to memoize-state approach.

[faceyspacey]

the fake dispatch is a great idea! The side effects would have to simply not be permitted. What do you mean by "way to inject 'additional' keys to track"?

...as for multiple components, we could do this:

static getDerivedStateFromProps(nextProps, prev) {
        const { dispatch, storeState, ...props } = nextProps
        const selector = prev.selector || memoize(sel)
        const state = selector(storeState, props) 

        if (!prev.selector) state.selector = selector
        return state === prev ? null : state
}

that way its memoized per instance. The codesandbox has been updated, and this works. I also added a comment about handling action creators:

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

...im starting to think you're right, memoizing action creators is a bad idea. I know I have apps that do dom manipulation side effects in action creators, etc. But maybe not, if it was clearly documented that its not allowed here. For ACs with side FX, you can simply pass the entire AC to your component, rather than pre-fill arguments in our selector.

[theKashey]

What do you mean by "way to inject 'additional' keys to track"?
If you will found, that memoize should also react on some keys it didn't before - ie set up tracking on the keys the usage of those it could not detect.

const selector = memoize((state,props) => () => props.dispatch(state.videos.length ? 1 : 2);
selector.setAdditionalTrackingOn(['.videos.length'],['dispatch']);

[faceyspacey]

but we don't need "key tracking" if we inject a fake dispatch, right?

...i guess we do if we wanted to allow arbitrary/any function, not just ACs. i think we can check that use case off. we explicitly disallow it.

[theKashey]

"the latest value provider" could solve any function case, but I'll better skip requirements and possibility of so black magic.

[theKashey]

So

• creating per-element selectors in getDerivedStateFromProps solves the issue with a single cache line depth,
• calling dispatches directly from component solves action memoization, but makes component less testable, as long it is become harder to understand that the action was dispatched.
• {strictArguments: true} is still the thing, as long if you will pass, but dont use props - memoization will be disabled. Currently redux will not even pass the props is selector does not accept them. Ie getDerivedStateFromProps should be disabled is selector accepts only the state.

I've played with your example, and it seems to be ok. It also removes areStatesEqual before the selector(as long memoize state is doing the same), and might could remove shallowEqual after(as long selector is a "whole" memoized), stripping away a lot of comparisons.

The only thing is missing - some trash dispatchers(a clock?) to stress test the approach.

[faceyspacey]

...yea, we can optimize it a lot still. and bring back to the arguments detection of the selector like in the original react-redux.

I think the next steps are:

1. not require additional shallowEqual check.
2. fake dispatch

Feel free to fork it and do anything you think would be good. It's basically based on how much time we have to incrementally improve it.

ps. your option is called strictArity currently :)

[theKashey]

1. You dont need it already. You just have to store the "result" of selector not AS state, but as a key in state. Look like react merges the state you generate, with the one component already had (setState is doing the same), resulting the new object.

static getDerivedStateFromProps(nextProps, prevState) {
        const { storeState, ...props } = nextProps
        const selector = prevState.selector || memoize(sel)
        const result = selector(storeState, props)

        return shallowEqual(result, prevState.result) ? null : { result, selector }
      }

Now you dont mix result with system information (ie "selector"), and maintain the "same" object across the changes.

2. is not the case, as long current version of selector did not pass dispatch down, thus removing a problem.
   In other case one will have to traverse result, searching for functions, and fake "somehow" dispatch (passed from props)? Sandbox execute it, get affected keys... and next rerun selector on each of those props change, even if you dont need to_. They will be needed only when you will click a button and emit an action, before then they better keep silent.

 const mapAllToProps = (state, {dispatch, ...props}) => ({
  name: state.name,
  action: () => dispatch({type:superActionAndAnaliticsEvent, payload:{state, props})
});

^^ that is legal, but will update on anything update.

[faceyspacey]

1. but the whole point is not to do shallowEqual because possibly you already know when selector is called, and the result is less work/comparisons. Do you have this info? If not, we can forget about this.

2. it can't update on "anything" as that defeats the purpose of high quality memoization that works for 80% of use cases. Now in 100% of use cases that dispatch state or props we have no memoization. We need a solution that does NOT update action function in all cases, except for when keys it needs change.

[theKashey]

1. The only thing to be changed - store the result of selector inside the state, not as the state. "State" is an uber object, controllable by React. A key in state - just a key, without magic.
2. mapStateToProps, to props be consumed by component by next render is one thing. To props to be consumed in action that might be never called - another. I don't see any other way, but distinguish mapState and mapDispatch as we always did before.

[faceyspacey]

1. but state.selector = selector cant make state become a different reference. selector should return the same reference (if it has the knowledge/info to do so).

2. it's a bit of extra unecessary work if it's never called, but it's the same thing as mapped state, just hidden.

[theKashey]

1. this.state = {key1:1}; this.setState({key2:2:}); this.state == {key1:1, key2: 2}. React merges states updated, thus it looses the reference equality. Just nextState = { selectorResult, selector }, not nextState = selectorResult. And it will work.

2. Huge unnecessary work. And I am not sure that executing every function we might found inside is a great idea.

What we could do with data adapters?

// allocate "persistent" object
const tracker = state.tracker =  state.tracker || {};
// store in that object "last" state
tracker.state = state;
 adapt = state => Proxy(state, {
    get(target, props) {
       // redirect "all" reads from to the "last" version
       return tracker.state[prop];
    }
 });
 const result  = selector(adapt(state))

But it will not work, as long breaks immutability of the state, and break all comparisons between the old state and the new, cos old state IS lifted to the new.
Achievable by some triggers around, for example - one could disable "lifting" while comparison is working, as long it should work only when someone "suddenly" trigger an action.

 adapt = state => Proxy(state, {
    get(target, prop) {
       if(global_noLifting) return state[prop]
       return tracker.state[prop];
    }
 });
global_noLifting=true;
 const result  = selector(adapt(state))
global_noLifting=false;

^ that already might work out of the box, only for top level keys, fast enought and even stable.

[faceyspacey]

1. ok, so we can do this?:

static getDerivedStateFromProps(nextProps, prevState) {
        const { storeState, ...props } = nextProps
        const selector = prevState.selector || memoize(sel)
        const result = selector(storeState, props)

        return result === prevState.result ? null : { result, selector } // no shallowEqual! yay -- last time, you still put shallowEqual here.
}

Then that task is done.

2. Yea, it's definitely a risky approach. I guess we can table this for now. If you think of anything else, let me know.

[faceyspacey]

code has been updated, and it works!

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

[faceyspacey]

I found an important selector use-case that does not work, but really should:

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

result === prevState.result is false always now

If we refactor it to the following it works:

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

So obviously it loses its reference capabilities when you destructure a portion of state. Is there any way around this?

[theKashey]

@faceyspacey - lets continue with spread in separate issue - #5