reduxjs / react-redux

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

Use Proxy-based selectors #1653

Closed theKashey closed 2 years ago

theKashey commented 4 years ago

Based on:


Proxy based solutions were floating around for a while, and actually actively used inside(MobX) and outside of React ecosystem(especially in Vue and Immer). I am opening this discussion to resolve status quo and add some momentum to Redux + Proxies investigations.

Overall proxies are capable to solve 3 objective:

// reading only .a and .b const fn = memoize(x => ({ sum: x.a + x.b, diff: x.a - x.b }));

fn({ a: 2, b: 1, c: 1 }); // ---> { sum: 3, diff: 1 } fn({ a: 3, b: 1, c: 1 }); // ---> { sum: 4, diff: 2 } fn({ a: 3, b: 1, c: 9 }); // ---> { sum: 4, diff: 2 } (returning old value) // ^ "c" does not matter



- __testing__. While the use case above is a little synthetic and such memoization is not really needed unless a developer willing not to use any memoization at all, it can be used to check _selector quality_, and report all selectors which react to the state change while they should not
> technically running selector twice in dev mode might solve majority of the problems, like returning new array/object every time, proxy based memoization is capable to detect more complicated situations and provide some guidance around the problem. As seen in `why-did-you-update`
![image](https://user-images.githubusercontent.com/582410/97410043-a9b29d80-1952-11eb-96f9-d37fdf5346ca.png)

- __slice isolation__. One named issue of redux is it's "global" store, and the fact that all listeners would be notified on state update. Knowing which slices were updated, and which selectors are listening for them, might provide an optimisation for selectors. This is actually a very old idea - https://github.com/reduxjs/react-redux/pull/1021 - but still valuable.

---

What proxies can solve:
- speed. Proxy based memoization works "more often" and might result a better experience
- less dev pain. Proxies "just works". At least, in the test mode, they can point on the problem instantly, not asking to invest some hours in debugging.
- TBD

---

What proxies cannot solve:
- per-component memoization. It's just absolutely orthogonal problem.
- TBD

--- 

What proxies can break:
- As long as one has to wrap `state` with a Proxy - one will use `Proxy` in all "frontend" code. In the same time at reducers/middleware side it will be just `state`. (Only once) I had a problem with "equal reference" memoization working differently in `useSelector` and `saga`. It's very edge-case-y, but having state sometimes wrapped, and sometimes not __can__ break stuff.

---

### Actionable items:
- [ ] decide about proxy-memoized selector
- [ ] decide about proxy-based selector pureness / quality checking
- [ ] decide about slice isolation or/and proxy-based tracking/DevTools integration. It's really possible to know which components reading which slice or/and react to a some piece of information and this knowledge can transform DevTools.

cc @markerikson , @dai-shi 
dai-shi commented 4 years ago

Does useTrackedState in #1503 fall into any of the three?

theKashey commented 4 years ago

@dai-shi - it is the first one. However I am trying not to focus on selection part as we did it already before, and it was not that easy to sell the it. The main problem with proxy-memoized selector is that you might not need it, as it not really solves memoization problem (due to cache size). For last 2 years I am using proxies to "check" my selectors in dev mode, and then use weakmap based memoization to band-aid the problem. I really don't have a case when you have to use proxies for memoization. Atoms (hello jotai), as well as normalized state, can do it better. The only difference - atomic "strings" and "numbers" has to be wrapped with weakmap-able objects.

markerikson commented 4 years ago

Thanks for putting this writeup together.

Lemme try to come at the question for a different angle:

What pain points of Reselect can be addressed by using a proxy-based selector approach, if any?

My thought process here is that perhaps the right place to start is by finding some scenarios where Reselect is a pain to work with, and proxy-based selectors solve those pain points. We can then look at maybe including something in RTK as a stepping stone away from Reselect, as well as revisiting the useTrackedSelector idea.

theKashey commented 4 years ago

Pain points of Reselect

Cache size

Proxies are not able to help here. There are ways to resolve the problem, but they are not bound to proxies.

Memoization

TrackedSelector can check that selector behaves in the same way as it should - be pure, and for the same "input data" produce the same "output".

It's again not bound to proxies or reselect, but only to how you write your code/selectors. There is always a way to produce a perfect memoization using reselect only.


In other words proxies are not going to magically solve all the problems, but they might lower the bar and improve dev experience and target product quality in many different ways. For example once I've created a POC for tracking Component->Redux->Rest. Ie I can know precisely which pieces of state were used to produce the result component (as seen at FromJS )

dai-shi commented 4 years ago

In other words proxies are not going to magically solve all the problems, but they might lower the bar and improve dev experience and target product quality in many different ways.

So, proxy-memoize v0.2 supports nesting and should help lowering the bar. Are there any examples that I can tackle applying the method?

finding some scenarios where Reselect is a pain to work with

pretty much this.

markerikson commented 4 years ago

Here's a few libs that specifically try to deal with weaknesses in Reselect:

Related:

I'd say those are good starting points for comparisons.

dai-shi commented 4 years ago

Thanks. So, I modified @josepot 's example in redux-views.

https://codesandbox.io/s/spring-star-1yrqm?file=/src/proxy-memoize.js

I noticed proxy-memoize would require useMemo/useCallback for parametric selectors.

markerikson commented 4 years ago

@dai-shi okay, yeah, the useCallback bit there is confusing both me and the linter :) I eventually figured out that the naming of that function collides with the React hook, so that's just a coincidence.

actual behavior-wise, is that ultimately just acting as a cache with size > 1?

tbh the redux-views example feels a bit more readable here.

dai-shi commented 4 years ago

the useCallback bit there is confusing both me and the linter

oops, I was trying to emulate the useCallback behavior in the example.

actual behavior-wise, is that ultimately just acting as a cache with size > 1?

uh, no. cache size = 1, but memoization is based on state and a prop (carId) separately. proxy-memoize-with-size.js is the example with cache size > 1.

tbh the redux-views example feels a bit more readable here.

Ha ha, I guess we should create react-redux examples which are common to use useCallback.

markerikson commented 4 years ago

@dai-shi oooo, yeah, I forgot that there was another example file there.

Hey, neat, it's got a size option built in! That right there is a big improvement over Reselect, and I guess you could just do size: Infinity if you wanted to?

Out of curiosity, is the separate variable read like const cars = state.cars necessary for the memoization/tracking to work right? Could I just do return state.cars.filter()? (Also, I assume destructuring works here too?)

dai-shi commented 4 years ago

you could just do size: Infinity if you wanted to?

Yeah, but that means memory leaks, and it does not solve the issue technically.

Could I just do return state.cars.filter()? (Also, I assume destructuring works here too?)

Yes.

proxy-memoize has a mix of WeakMap cache and Array (with size) cache.

  1. firstly check weakMap.has(obj) without tracking info (so, { state, carId } always misses.)
  2. if it doesn't find, check the array cache with tracking info
  3. otherwise, run the function and record tracking info
markerikson commented 4 years ago

Okay, so here's a use case I'm not clear on yet.

Let's say I've got:

const selectTodoDescriptions = createSelector(
  state => state.todos,
  todos => todos.map(todo => todo.text)
)

Now I dispatch(toggleTodo()). New todo object, new state.todos array. We call selectTodoDescriptions(state), todos has changed, output selector runs, and returns a new todoDescriptions array that is a new reference but shallow-equal because none of the todo.text values changed.

Is proxy-memoize going to do the same thing, because state.todos changed? Or is it smarter in that case in some way, because we only read todo.text and the new todo object only has a new todo.completed field?

dai-shi commented 4 years ago

This is rather easy, because we don't even need nested memoize.

const selectTodoDescriptions = memoize((state) =>
  state.todos.map((todo) => todo.text)
);

In this case, the tracking info is something like: state.todo[0].text, state.todo[1].text, state.todo[2].text.

Example: https://codesandbox.io/s/quirky-austin-yxnts?file=/src/index.js

markerikson commented 4 years ago

:eyes: :eyes: :eyes: 💥

okay, see, THAT right there is an immediate win over Reselect!

And the fact that this has a size option built in means you could at least give yourself a bigger cache right away and be able to skip per-instance selectors, right? ie, selectTodoDescription({state, todoId}), give it a size of 500 or something? (okay, bad example because that's not transformed data so a direct lookup is okay, but you know what I mean here)

dai-shi commented 4 years ago

👍 👍 👍

selectTodoDescription({state, todoId}), give it a size of 500 or something?

Yeah, that's correct.

But, if it's used in react-redux, my suggestion (and that's why I used useCallback mock in the previous example) is:

const selectTodoDescriptions = memoize((state) =>
  state.todos.map((todo) => todo.text)
);

const Component = ({ todoId }) => {
  const selector = useCallback(memoize((state) => {
    const descriptions = selectTodoDescriptions(state);
    // do something with todoId
    return some_results;
  }), [todoId])
  // ...
};

(btw, react-hooks eslint rules can't handle this pattern, so we may prefer useMemo to useCallback.)

theKashey commented 4 years ago

selectTodoDescription({state, todoId}), give it a size of 500 or something?

🤔 what if one can provide a cardinality function, itself memoized as well, to calculate the optimum capacity for a given function?

markerikson commented 4 years ago

@theKashey not quite sure what you're suggesting there or how it might work, tbh.

theKashey commented 4 years ago

So we are here are trying to resolve a few underlaying issues:

selectTodoDescription({state, todoId}), give it a size of 500 or something?

Could be a solution for 2 and 3. Just provide cache side for a selector

// please imagine that selectTodoDescriptions selects something based on props
// ie needs more that 1 cache size. 
const selectTodoDescriptions = createSelector(  
  selector,
  combiner,
  500, /*or something*/
)

Well, that would not work as cache side is not a constant, but a slight change can ease

const selectTodoDescriptions = createSelector(
  selector,
  combiner,
  state => /*🤷‍♂️*/ state.todos.length, // this is how many cache lines you need.
)

Proxy based selectors, which uses selected keys as cache index are capable to efficiently use provided cache, so this pattern can work. Not sure it will work efficiently, as cache look up in terms of proxy-memoize is a VERY expensive operation - you really have to do something, compare keys and do deep object property access. Pretty sure that with a cache size 500 the look up operation (Omax == O(500), never forget about the constant part) would reduce all benefits of such approach. However we can take a step back and see what re-reselect does

const getUsersByLibrary = createCachedSelector(
  // inputSelectors
  getUsers,
  getLibraryId,

  // resultFunc
  (users, libraryId) => expensiveComputation(users, libraryId),
)(
  // Use "libraryName" as cacheKey
  (_state_, libraryName) => libraryName
);

The problem here is libraryName which causes a memory leak. How it wish it be an object I can weakmap to store the cache into... well, why not?

const getUsersByLibrary = keyedMemoize(
  // inputSelectors
  getUsers,
  getLibraryId,

  // resultFunc
  (users, libraryId) => expensiveComputation(users, libraryId),
)(
  /* cache size, can be >1, but still <500 */
  10,
  /* "key object", a weakmappable target to store cache in */
  (_state_, {libraryName}) => _state_.libraries[libraryName], 
);

Now cache in bound to data, not React Components, which is 👍(data it source of truth) and 👎(actually we need memoization for UI) in the same time.

const selector = useCallback(memoize((state) => {
    const descriptions = selectTodoDescriptions(state);
    // do something with todoId
    return some_results;
  }), [todoId])

⬇️⬇️⬇️⬇️

const selector = keyedMemoize((state) => {
    const descriptions = selectTodoDescriptions(state);
    // do something with todoId
    return some_results;
}, {
 cacheSize: 1,
 key: (state, {todoId}) => state.todos[todoId],
});

Now key-ed proxy memoized selector can be extracted out of React Component 😎

markerikson commented 4 years ago

Some interesting thoughts and examples there. The "keyed" example looks an awful lot like what I remember seeing from redux-views.

So, what would need to be added to proxy-memoize to enable something like that?

dai-shi commented 4 years ago
const selector = keyedMemoize((state) => {
    const descriptions = selectTodoDescriptions(state);
    // do something with todoId
    return some_results;
}, {
 cacheSize: 1,
 key: (state, {todoId}) => state.todos[todoId],
});

This looks interesting. If we are sure the key is a stable object, we should be able to use WeakMap to cache memoized functions?

import memoize from 'proxy-memoize';

const memoizedFns = new WeakMap()
const keyedMemoize = (fn, key) => {
  if (!memoizedFns.has(key)) {
    memoizedFns.set(key, memoize(fn));
  }
  return memoizedFns.get(key)
};
markerikson commented 3 years ago

Okay, here's another weird edge case I'm curious about.

Reselect selectors and Immer's Proxy-wrapped draft states don't get along well - the selectors think the Proxy ref is the same, so they don't recalculate results correctly.

Over in https://github.com/reduxjs/redux-toolkit/pull/815 , we're planning on adding a createDraftSafeSelector API to RTK that just checks if the first arg is an Immer draft, and if so, calls current(draft) to force the selector to recalculate.

What happens with proxy-memoize in that scenario?

dai-shi commented 3 years ago

The current proxy-memoize is designed for immutable state model, so it doesn't help. If you pass a ref-equal draft, you get the cached result. This is to get better performance and for cache size issue.

We could add an option to disable the immutability optimization and always compare all affected properties. But, I'm not sure how useful it would be.

const memoizedFn = memoize(fn, { mutable: true }) // not yet implemented
markerikson commented 3 years ago

Yeah, I figured that was probably the case.

I suppose it might be nice to have that as an option, but dunno how much more complex it makes the internal implementation.

theKashey commented 3 years ago

"Immutable Proxies" are reflecting the "immutable object"(State) underneath and change when that object changes. The same is about immer(as a part of structural equality) - only affected paths would be changed.

👉 That is the game rules

And when you are changing your state in reducer, and passing an "incomplete" state to selectors - you are bypassing these rules. Calling current(draft) seems a good way to make an operation according to these rules, but let's focus what will happen you don't do that:

The missing puzzle piece here is a universal "proxy marker"(a Symbol), to indicate to any underlaying logic - "I am a Proxy, transform me if you need". This is how proxy-memoize can current(draft) Immer's proxy, and reselect can unwrap Proxy from proxy-memoize, and so on.

And look like @dai-shi is the best person to introduce such marker to the Proxy-based ecosystem. Might be one will need more than one marker, right now I can think only about "acces to the underlaying state"/"immutable" one.

dai-shi commented 3 years ago

Something like proxy[Symbol.for("proxy.unwrap")] ?

(but they would work slightly differently... also unwrap doesn't mean to return immutable one...

theKashey commented 3 years ago

Something like it 😉. Actually, it's not quite about unwrapping, but closer to current - here is the object you should work with instead of me, please use it. Another problem is "when" you should call it - obviously not every time, but only when reference is passing through some "boundaries", like immer->(not here)redux->(here!)proxy-memoize.

dai-shi commented 3 years ago

Actually, it's not quite about unwrapping, but closer to current

Right, that's what I assumed. proxy[Symbol.for("proxy.immutable")] might be better.


const memoizedFn = memoize(fn, { mutable: true }) // not yet implemented

It turns out that this is not trivial. It essentially needs to copy the object (at least partially) when it compares. As immer has current, we should use it which does basically the same job.

markerikson commented 3 years ago

so lemme ask this: how "done" / "ready" is proxy-memoize at this point? I have faith that it basically works as advertised, so the outstanding questions are:

I just wrote up a big comment in Reactiflux about the behavior of Reselect vs proxy-memoize, so I'm definitely interested in trying to start advertising proxy-memoize as a recommended option - at a minimum on my blog, and potentially in the Redux docs shortly after that. I'd just like to know that it's sufficiently stable before I do so.

dai-shi commented 3 years ago

Apart from selectors with props and cache size discussion, which would still be controversial, the basic feature is complete, I'd say.

How many known edge cases or bugs does it have?

Known edge cases I can think of is when a selector returns something that we can't untrack. For example, if a selector returns a Set that contains a proxy value in it, we can't unwrap the proxy.

How can those avoided / fixed / worked around?

We should recommend a selector to return primitive values and plain objects/arrays (nesting is ok.)

What other features or API changes would be useful to have before calling it 1.0?

The major concern of mine is not many people are using it and there can be unknown edge cases. It's a chicken and egg problem. What would be useful before 1.0 is to collect bug reports and clarify limitations (or possibly fix them.)

start advertising proxy-memoize as a recommended option

Would be super nice. So, I'd say it's ready as I covered all what I have noticed. But, technically, it's more complex than reselect, and there can be unknown issues.

markerikson commented 3 years ago

Cool.

Tell you what - could you update the readme with that sort of info on status and workarounds, and add some additional docs examples based on our discussions in this thread? In particular, I think some comparisons of some typical Reselect use cases and how they'd look with proxy-memoize instead (and maybe even one or two for Re-reselect and Redux-views?), to show when/how/why proxy-memoize works better.

I'm going to find time over the Christmas break to update my existing "Reselect selectors" blog post to cover useSelector and proxy-memoize. I'll then try to convert that into a new Redux docs page that I might put into both the Redux core and React-Redux docs.

theKashey commented 3 years ago

There is only one big problem with proxy-based solutions - they wrap the original object. There a few consenquences:

There is also a few more "good to have"s, which will require some changes from redux side:

markerikson commented 3 years ago

yeah, I can see a use case for wanting to occasionally debug values inside of selectors. This goes back to the "unwrapping' discussion we had earlier. @dai-shi , is there an easy way to implement a version of current for proxy-memoize?

@theKashey yeah, I know you've been poking at the idea of double-rendering mapState/useSelector for a while :) Agreed that there's potential value there, but that's a separate topic. Even though we ended up with this issue in the React-Redux repo, right now my goal is just to make sure that proxy-memoize is good enough that I can officially recommend it alongside (and ideally preferred over) Reselect. That's independent of how React-Redux behaves.

nathggns commented 3 years ago

I just wrote up a big comment in Reactiflux about the behavior of Reselect vs proxy-memoize

@markerikson would you be able to link to this comment for myself and other travellers?

dai-shi commented 3 years ago

It might be a good idea to add some dev mode checks that proxies are not accessed when they are not "ready"(ie outside of proxy-memoize)

This would be technically possible for proxy-memoize as it knows when it's finished. The implementation might not be trivial, though.

@dai-shi , is there an easy way to implement a version of current for proxy-memoize?

This should be easy as it already has an upstream api. A small hesitation of mine is not being sure how to export it as an api of proxy-memoize and how to document it.

How would a user use such a util function to unwrap a proxy?

markerikson commented 3 years ago

@nathggns as I said, I'm going to be updating my existing blog post at https://blog.isquaredsoftware.com/2017/12/idiomatic-redux-using-reselect-selectors/ with that additional material over the break, as well as trying to add a couple docs pages based on that post.

markerikson commented 3 years ago

@dai-shi I would assume that it'd be similar to what you do with current right now:

const todosSlice = createSlice({
  name,
  initialState,
  reducers: {
    todoToggled(state, action) {
      const todo = state.find(t => t.id === action.payload);
      // temporary code while I'm debugging
      console.log(current(todo));
      t.completed = !t.completed;
    }
  }
})

so, hypothetically:

const selectScore= memoize(state => {
  const intermediateResult = heavyComputation(state.a + state.b);
  // temporary code while I'm debugging
  console.log(unwrap(state.c));
  return {
    score: intermediateResult,
    createdAt: Date.now(),
  }
);
dai-shi commented 3 years ago

@markerikson For a simple console.log use case, I don't think we need to unwrap a proxy, unless we want to completely hide the proxy to developers. It will display like this in Chrome dev tools.

image
  // temporary code while I'm debugging
  console.log(unwrap(state.c));

You probably mean unwrap(state).c? For this use case, yeah, we'd need to export an api something like unwrap. I will work on it.

markerikson commented 3 years ago

actually, yeah, the "[[Handler]]" stuff is part of what I'd want to hide. That's confusing for people.

I think I was assuming that .c is wrapped in another proxy and I'm trying to unwrap that one, but yeah, you get the idea.

dai-shi commented 3 years ago

I think I was assuming that .c is wrapped in another proxy and I'm trying to unwrap that one

Yes, that's correct. .c is wrapped in another proxy and needs unwrapping to console.log.

Let me remind this just in case, as it's important: Doing console.log(state.c) without unwrapping records ".c" as used, and it will re-run the function when state.c is changed. So, the behavior will be different with and without this console.log statement. console.log(unwrap(state.c)) is the same thing.

markerikson commented 3 years ago

Hmm. Okay, yeah, that might be another gotcha to note.

dai-shi commented 3 years ago

@markerikson README updated: https://github.com/dai-shi/proxy-memoize

I think some comparisons of some typical Reselect use cases and how they'd look with proxy-memoize instead (and maybe even one or two for Re-reselect and Redux-views?)

I did what I could. I'd need someone to help on this to make them better...

markerikson commented 3 years ago

Yeah, I'll find time in the next few days to play with this and help update the examples.

dai-shi commented 3 years ago

This is more related with #1503, but as this thread is more active and it is slightly related, let me post here. I just released react-tracked v1.6.0 which exports createTrackedSelector. This allows to create useTrackedSelector from useSelector.

import { useSelector } from 'react-redux';
import { createTrackedSelector } from 'react-tracked';

const useTrackedSelector = createTrackedSelector(useSelector);

Now, this works pretty similar to useSelector + proxy-memoize.

const getTotal = memoize(state => ({ total: state.a + state.b }));

  // in component
  const { total } = useSelector(getTotal);

  // equivalent version
  const state = useTrackedSelector();
  const total = state.a + state.b;

The major difference is useTrackedSelector returns a proxy, but memoized function unwraps proxies on return.

The useTrackedSelector here is equivalent to useTrackedState in #1503. I just name it differently to avoid confusion. What this means is that we can get the same behavior opt-in, without merging the PR.

For working examples, check the example code and codesandbox at: https://react-tracked.js.org/docs/tutorial-redux-01

markerikson commented 3 years ago

Sorta related, either of you ever seen https://github.com/pzuraq/tracked-redux ?

dai-shi commented 3 years ago

I'm not familiar with Ember ecosystem at all, but it looks like it is for @glimmer/tracking which seems to be built on a different model. There can be something to learn though.

markerikson commented 3 years ago

Yeah, it's definitely built on Glimmer's auto-tracking system, but my (vague) understanding is that that tracking system is UI-agnostic in much the same way Immer is (or Vue 3's new reactivity system).

dai-shi commented 3 years ago

If I understand it correctly, mobx, vue, glimmer all base on mutable state.

// mutable state
const state = { a: 1 };
state.a // mark as used (pseudo code)
state.a += 1
// ".a" is still marked as used.

// immutable state
let state = { a: 1 }
state.a // mark as used (pseudo code)
state = { ...state, a: state.a + 1 }
// the used mark is gone for the new state.

I believe this behavior is important for hooks composability and Concurrent Mode.

theKashey commented 3 years ago

Interesting. So in terms of "tracking" mutable and immutable data structures differ more on boundaries and time. Like there are no boundaries/generations with mutable ones. Wondering how named state managers are solving predicability part.

From another point of view - it might be an interesting task to transfer usage information from one state to another. Is there any use case when it would be needed? Right now we are doing it on "cache hit", however it could be required for selectors to prevent usage flakes.

"Usage flakes" are when more than one selector are assessing state, and at least one of them does "cache hit" and NOT accessing nested keys. That results the second run of memoized selector to produce might be the same value, but different usage information, which changes the way tracking works and alters the third run, when proxy would consider higher values as "important".

I have a feeling that @dai-shi tackles this somehow, like I've seen the code somewhere.

dai-shi commented 3 years ago

I have a feeling that @dai-shi tackles this somehow, like I've seen the code somewhere.

Yeah, that was really tricky with nested memoized selectors. https://github.com/dai-shi/proxy-memoize/blob/84878e7d21adf9482b63e187751e8b5acf9fba04/src/index.ts#L76 This touchAffected copies the usage info (= affected) form the inner memoized selector to the outer memoized selector. I mean, I don't test it with various cases, but it's the intention to support such cases. It's not super efficient if affected is very big.

markerikson commented 2 years ago

Hiya, folks. At this point, I don't think we're likely to add a specific new proxy-based selector API to React-Redux. We do have a mention of proxy-memoize in https://redux.js.org/usage/deriving-data-selectors , and if we ever get around to adding a selectors page to the React-Redux docs I'm happy to highlight it there as well.

I'm going to go ahead and close this.