Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for per-scope selector instantiation #99

Closed
anacierdem opened this issue Oct 13, 2020 · 17 comments
Closed

Add support for per-scope selector instantiation #99

anacierdem opened this issue Oct 13, 2020 · 17 comments
Labels
enhancement New feature or request

Comments

@anacierdem
Copy link

anacierdem commented Oct 13, 2020

When creating a hook, it is possible to provide a selector. Consider this contrived example which will just select a property on the store;

const useValue1 = createHook(Store, {
  selector: (state) => state.value1 + state.value2,
});

When this hook is used the selector will get executed with the up to date state and return the required value. In the docs, it is also mentioned that we can use memoized selectors (e.g reselect). The following example will not execute the (value1, value2) => value1 + value2 selector unless the output of value1Selector or value2Selector (which might also be memoized) changes;

const useValue1 = createHook(Store, {
  selector: createSelector(
    [value1Selector, value2Selector],
    (value1, value2) => value1 + value2
  ),
});

The problem with this is that we are "creating" a selector once when we are creating the hook. If we use this selector with multiple instances of the store (say different containers) the memoization will loose its effectiveness as the single level of memoization will think the values has changed whenever a new mutation (with a potentially different store state) is initiated one after another. (Also see this)

To be able to utilize selectors at their best, we need a way to instantiate selectors together with store instances and this requires accepting a selector "creator" that will get executed once for each scope.

const useValue1 = createHook(Store, {
  // This function will get executed and the resulting selector is tracked internally when necessary
  selector: () => createSelector(
    [value1Selector, value2Selector],
    (value1, value2) => value1 + value2
  ),
});

It maybe ok to have another option named selectorCreator, but this may make the API a little confusing for future users. Changing the default behaviour on the other hand is probably not a good idea.

@anacierdem anacierdem changed the title Add support for per-container selector instantiation Add support for per-scope selector instantiation Oct 13, 2020
@albertogasparin albertogasparin added the enhancement New feature or request label Oct 25, 2020
@albertogasparin
Copy link
Collaborator

Indeed you are right: when using containers a reselect selectors are basically useless. The truth is that even in large applications, given sweet-state stores are generally smaller than redux, we saw a reduced need for such deep selectors optimisations.
Moreover, sweet-state memoises them by default at hook level, which already provides some level of optimisation.

But I understand that sometimes you might have more complex needs, and providing a pattern for it is probably good. I wonder if you can achieve that by storing the createSelector in state and call it in the hook selector (so it will be bound to current scope). Probably a bit hacky though 🤔

@anacierdem
Copy link
Author

anacierdem commented Oct 28, 2020

Sweet state indeed memoizes the return value of the selector and if that does not change, will not trigger a re-render. On the other hand, think about a simple operation where we have id indexed objects in the state and we want to display them in specific order so that in the selector, we create a new array every time the selector gets executed. As we will be creating a new reference each time, the optimisation does not apply for this case.

I'm not sure if I get the proposed solution though. We need to keep the return value of createSelector on the state and call that as the selector. One way of doing that is maybe creating the selectors on Container's onInit, keeping them on the state and calling them in the component. This will make it possible to use the selectors in actions too, sparing a few computer instructions there.

This is an initial attempt (not tested):

const MyContainer = createContainer(Store, {
  onInit: ({setState, getState}) => {
    const firstSelector = createSelector(...);
    const secondSelector = createSelector(...); // Here we can use firstSelector too
    // This part can be automated to some degree, or we may have a useSelector hook
    setState({
      selectFirst: () => firstSelector(getState()),
      selectSecond: () => secondSelector(getState())
    })
  }
});

const MyComponent = () => {
  const [{selectFirst, selectSecond}] = useStore();
  const firstValue = selectFirst();
  const secondValue = selectSecond();
  return ...;
}

The disadvantage is that we now need to manage additional state for each selector we have, which might get automated to some degree. We are already using a wrapper around create... methods, so we can accept an additional object for selectors and bind them in the onInit callback by default. Still I think whole of this can be implemented in the core, with the additional benefit of no-update-if-selector-output-not-changed guarantee. For the above example the output array from the selector will stay the same if the id indexed state and ordering criteria remains the same.

@albertogasparin
Copy link
Collaborator

Well, what I was suggesting was just using the store to hold the instances:

const MyContainer = createContainer(Store, {
  onInit: ({setState, getState}) => {
    setState({
      selectors: {
         first: createSelector(...),
         second: createSelector(...),
      }
    })
  }
});

const useBla = createHook(Store, { 
  selector: (state) => state.selectors.first(state)
});

I agree that having such functionality built-in could be handy, but it needs to be well thought through, both on implementation an behaviour. The current selector implementation is far from perfect (as you have highlighted) so I would be considering changing it (for instance deprecating selector and pushing for selectorCreator) but needs to be rock solid this time.

Because it's not just about adding new code. There are subtleties that I'd like to get right before extending the library.
For example, if you provide a selectorCreator option where should the instance be bound to? For your use case, I get you'd like it to be state (scope) based, so consuming the same selector/hook in various places under the same scope returns the same value. However, if I add props/args to the selector, then it becomes useless again as it will get recomputed every time (as today with scopes) and to be effective the selector instance should be bound to the component/hook itself. But that would reduce the performance in many other cases.
So which scenario is more common? Which behaviour produces less surprises?

Should sweet-state instead provide a custom version of createSelector that deals with such nuances so you can create them globally, by scope and by hook instance? As maybe reselect design is not fit for what we need 🤔

@anacierdem
Copy link
Author

Actually, similar problems are also there when using redux with hooks (see bottom example at https://react-redux.js.org/next/api/hooks#using-memoizing-selectors) and they decided to leave that control to the consumer via an additional useMemo. I don't think the state library should handle those cases (for hooks at least) either, but for the state that is "owned" by the library, it should provide some guarantees. So IMHO, selectorCreator should run given selector for the current scope similar to the example you have provided.

On the other hand for RSS, probably your main concern is not any prop in the react app but rather container props, which will cause a cache invalidation for every successive container render too and this is also must be addressed in the library, I agree. Then we can bind both to the state and to props like;

const useBla = createHook(Store, { 
  selector: (state, containerProps) => state.selectors.first(state, containerProps)
});

onInit is firing for every Container even if they have the same scope similar to #97 so with this method we are already creating new selectors but storing them on the same keys, invalidating each other. This requires another set of state values for each individual container and increase the onInit and selector logic quite a bit (We need to maintain the relation between container instances and selectors, which I doubt may not even be possible inside onInit). Is this your performance concern if this is implemented in the library?

reselect is not something very opinionated IMHO, any memoization attempt will have similar problems as long as we do not have a way to create independent scopes for each of them.

@albertogasparin
Copy link
Collaborator

albertogasparin commented Nov 8, 2020

Hey @anacierdem, I've been playing around a bit with selectors. I think I found a good compromise between simplicity and effectiveness. Please, if you have time, give it a go by installing react-sweet-state@next (you can see the changes here and testing it out).

Basically I'm not changing the current API, but I'm improving createSelector support (and also exposing a version better suited for sweet-state then reselect without requiring another dependency, but it's optional). So:

const useValue =  createHook(Store, {
  selector: createSelector(
    [value1Selector, value2Selector],
    (value1, value2) => value1 + value2
  ),
});

will automatically get "optimised" now (no need for consumers to change anything, yey!). createSelector will create an instance, but Sweet-state instead of using it directly will generate individual instances from it for each hook/component, or per scope when conditions allow.

This means that useValue can be used across multiple scopes and components and they will not invalidate each other memoisation. However, given handling memoisation on a global/store level poses some challenges, and that different arguments can be passed to useValue in different locations, I've decided that only hooks without arguments will have selectors cached per scope, while hooks receiving any argument will have the selector bound to that hook instance.

useValue() // on mount: called, on state update: called
useValue() // on mount: cached, on state update: cached
useValue(1) // on mount: called, on state update: called
useValue(1) // on mount: called, on state update: called

Deoptimising when we use arguments it is not ideal, but the impact of potentially cross invalidating memoisation is worse than recomputing when needed. I read several discussions about "shared-selectors" on reselect repo (like for v5) and checked some experimental implementations (like re-reselect) but they end up requiring devs to provide a cache key.
Explicit cache keys are powerful but also quite error prone and they leave questions like cache cleanup unsolved, so I like that Sweet-state new approach can get most of the benefits without requiring devs to think.

cc @theKashey

@anacierdem
Copy link
Author

I just realized you were referring to the hook arguments as props, it makes more sense to me now 😅 I didn't know reselect was exposing the selector structure upon doing createSelector. Although this is a reselect focused solution (won't work with a simple memoized function) it seems acceptable as a compromise indeed, especially if r-s-s ships with its own reselect compatible set of memoization helpers. This will be a huge plus even without arguments support as it will enable proper update guarding for simple cases like filtering/sorting a list.

I'll give that a try when I have the time for sure. I wonder if we could find a nice to use api to also inject the selectors to actions so that we can utilize the pre-calculated value there too 🤔

@anacierdem
Copy link
Author

anacierdem commented Nov 8, 2020

Haven't tested this yet but I just realized we may also need to recursively create selectors o/w the nested selectors will get invalidated for changing scopes. For example for a selector A depending on B and C, createSelectors output will first invoke B and C and they will not be instanced selectors as they have been created before r-s-s has stepped in. Disregard if there is something that I overlook in the current impl.

@albertogasparin
Copy link
Collaborator

Yeah, I didn't add support for nested selectors optimisation. I'll probably add a note saying that they are discouraged. While I recognise that createSelectors can be useful for mapping and filtering values out of a store, I question the need for much more complicated selectors, like nested ones.
Indeed, I believe selectors facilitate a wrong store architecture, as they push to build megastores (a la redux) instead of decomposing them into multiple independent pieces, and that goes agains Sweet-state principles.
So, while they are a great to optimise the performance on critical parts of the app, the store shape should be simple enough to not need the hundreds selectors you would have with redux, and selectors reuse should not even be a thing.

@theKashey
Copy link

theKashey commented Nov 9, 2020

Memoization is a real rabbit hole, with the proxy-based selectors on the technology frontline.

Honestly speaking I haven't seen any per-instance memoization working out of the box without some flaws. At least it will be a hard switch between sharing memoization result between all callers (useValue), and not (useValue(1)).
Technically "double memoization" is the easiest way to kill two birds, mentioned above, with one stone.

createSelector will first read data from two sources - "global" and "per instance", and then write to the same targets.

  • in case you don't need per instance memoization "global" cache will always work fine
  • in case you need - global cache will be evicted every time, but per instance, cache will keep the instance alive.

Anything else, like weakmap based cache, babel based magic and so on might not that great, and may require API changes as well.

However, two years after creating kashe, it's still the only memoization pattern working for me. Well, sometimes it's become tricky to find a right medium to store the cache.

The new selectors are based on the same idea, but please consider saving the result to some "global" cache as well.

@anacierdem
Copy link
Author

Yeah, I didn't add support for nested selectors optimisation. I'll probably add a note saying that they are discouraged. While I recognise that createSelectors can be useful for mapping and filtering values out of a store, I question the need for much more complicated selectors, like nested ones.
Indeed, I believe selectors facilitate a wrong store architecture, as they push to build megastores (a la redux) instead of decomposing them into multiple independent pieces, and that goes agains Sweet-state principles.
So, while they are a great to optimise the performance on critical parts of the app, the store shape should be simple enough to not need the hundreds selectors you would have with redux, and selectors reuse should not even be a thing.

I agree that r-s-s stores should be small, independent structures, it generally makes things more manageable, I agree. On the other hand I don't think selectors are the cause of a wrong store architecture. Instead I believe they provide the tools to make store simplification/normalization easier as you no longer have to think about performance and such, and can focus on the store shape itself.

To have a nested selector structure, we don't need to have hundreds of selectors, consider the simple case where we want to filter/sort a list twice; we may want to display items in a particular order and at the same time need to highlight first N items matching a criteria in a separate ui view (e.g display a few favorites on a side panel). In this case we would like to re-use the sorted list in order not to repeat the code on both selectors and we would simply nest the two selectors. I'm pretty sure there will be similar simple use cases that may justify nested selectors, which in this case will cause re-updates whenever the store is updated.

If you really think such uses cases should not be a concern for sweet state, that's ok too. It is just not the way I think it should be :)

@anacierdem anacierdem reopened this Nov 9, 2020
@albertogasparin
Copy link
Collaborator

Thank you for providing more examples and context. It really helps out understanding the needs and potential workarounds / solutions / guides to consumers 😊

Unfortunately I don't see how nested selector can be properly supported, as the same issue affects reselect in redux itself: either you create the selector in mapStateToProps (so local) or you create it outside (so global). But you cannot mix and match otherwise that would break memoisation.
Don't see any API on sweet-state level that can allow you to create selectors locally (or per scope) and still share them between hook creators 🤔
For that I would probably suggest a more powerful selector library that handles multi-layer cache internally.

But if you have some ideas, shoot! This is the right time as nothing has been finalised yet

@anacierdem
Copy link
Author

I'm not sure I get the problem with nested selectors on redux. It is possible to create all the selectors inside mapStateToProps as local using selector creators, we don't need to mix and match global and local ones. They will be all local indeed. Redux allows returning a function from the map state to props function so we have the opportunity to create whatever local selectors we need.

const makeMapStateToProps = () => {
  const getListA = makeGetListA();
  const getListB = createSelector([getListA], nestedSelector);
  const mapStateToProps = (state, props) => {
    return {
      listB: getListB(state, props)
    }
  }
  return mapStateToProps
}

It will be the same for r-s-s if it supports nested selectors. I don't think we need to share them among creators (or I do not have a use case for that in mind).

The last thing I think might be helpful is the ability to use the scope-local selectors in actions too. I'm just thinking out loud;

// Register some selectors for the store;
const Store = createStore({
  initialState: ..,
  actions: ...,
  selectors: () => {
    // Here we can instantiate the selectors per-scope
    return {
      myValue1Selector: createSelector(...),
      myValue2Selector: createSelector(...),
    }
  }
});

// When creating the hook;
const useMyValue1 = createHook(Store, {
  selector: 'myValue1Selector', // This may enable a backwards-compatible alternative although I didn't like using a string key here.
});

// Then in actions;
const actions = {
  myAction: () => ({ getState, dispatch, selectors: { myValue1Selector, myValue2Selector } }, { api }) => {
    // This is not executed unless we explicitly do so;
    const mySelectedValue1 = myValue1Selector(getState());
    // Or maybe a shorthand;
    const mySelectedValue2 = getState(myValue2Selector);
  },
};

@theKashey
Copy link

By creating selectors in mapStateToProps, also known as mapStateToPropsFactory, you can "fork" only first level of selectors. As long as selectors tend to use some sort of hierarchy - it's not helping at all. The "cacheSize=1 rule" will break memoization in some other place.

There is a way to make this moment better (but not solve):

  1. what does (reselect's) createSelector do? Long story short - only creates local scope variable to store the call result. You don't need to create new selector every time if you can control that cache from the outside
  2. what you lose if you have per-instance memoization? You lose global cache. But if you can control the "cache" from outside, then you can provide another object, with the same interface, which might "do more". Like read from and write to more than one medium. See my comment above about how it helps solving per-instance+global memozation.
  3. is double store leads to double memory usage? Hopefully no. This is almost safe to use.
  4. However is you have many components selecting let's say just two, but different branches of the state - memoization will be always per-instance, not shared between similar instances. And usually that's ok, as we need equal-reference based checks mostly on "per-instance", and memoization(in React) is not trying to skip the actual memoized operation, but mitigate cascade updates after it.
  5. So you might not need global cache at all (depends on your use case)

In other words - all you need is to control this variable and set a proper value before any call to any selector, which should not be a big deal as any access goes though a hook.

@anacierdem
Copy link
Author

@theKashey I think, nesting depth should not matter if we don’t want to re-use the cache among instances - i.e we will have N independent hierarchies with internally consistent and valid memoization. This is probably what you refer as “loosing the global cache” - keeping multiple copies of the potentially same values and doing updates unnecessarily from time to time until all of them stabilize. It is sub optimal, but should be perfectly safe for different instances to have their own copies of the cache. If we want to further optimize and re-use the cache as much as possible, yes we will need some way of injecting two different versions, one for per-scope and one for per-instance. If any of the two is a match, we can use the respective stored value. I think this is your point am I right?

@albertogasparin
Copy link
Collaborator

Thanks for the input guys.

It is possible to create all the selectors inside mapStateToProps as local using selector creators ... I don't think we need to share them among creators
The last thing I think might be helpful is the ability to use the scope-local selectors in actions too.

These sounds legitimate use case for some deep optimisation, however I agree with @theKashey that "memoization(in React) is not trying to skip the actual memoized operation, but mitigate cascade updates after it" so if the selector runs again it should not be a problem. And the default Sweet-state behaviour (of shallow compare even the output before letting an update fire) will already stop many scenarios from triggering a re-render, like in the example of favourites on a side panel.

Interestingly, the more we get on with this discussion, the more I get the feeling that what you are really asking @anacierdem is a way to store a number of derived state somewhere. Which is a superset of what selectors are meant for (avoid unnecessary re-renders), and I think the solution is in computed properties (ember-data was king in this space).

Computed properties might look similar to selectors but they are more explicit and constrained, given that they are attached to the store state (or model if you are familiar with ember-data) instead of being bound to the hook/subscriber (as selectors are). Going back to one of my first answers, I believe the pattern below is actually much better compared to abusing selectors in hooks/subscribers:

const MyContainer = createContainer(Store, {
  onInit: ({setState, getState}) => {
    setState({
      computed: {
         sortedList: () => {}, // an implementation
         favouteList: () => {}, // ...
         ...
      }
    })
  }
});

Building a small util that does that for you should not be hard, and features like having those available in actions, and hooks consuming the proper cached value, work out of the box:

const actions = {
  myAction: () => ({ getState }) => {
    // This gets an already computed value if still fresh
    const favouteList = getState().computed.favouteList();
  },
};

const useFavoteList = createHook(State, { selector: s => s.computed.favouteList() })

I think a production ready version of this pattern (and util) could be a great recipe to add to Sweet-state docs, as several people might find it useful. Probably one of the small libs out there makes creating memoized computed properties a breeze, using proxies and other smartness to reduce even more invalidation (or reselect might work fine too).

I start to think that the complications arise when we try to combine state selection with derived data computations. reselects tried to solve both, but we ended up with a complex API, with hard to track deoptimisations and memoisation issues. Splitting the two concerns might be more verbose but could lead to a much easier to understand and reliable API.

From Sweet-state perspective, what could be added to simplify the support of such patterns is allowing initialState to be a function (that could be an easy thing to add API wise) so there is no need for a Container onInit to create the properties bound to an individual store state.

@anacierdem
Copy link
Author

anacierdem commented Nov 10, 2020

"memoization(in React) is not trying to skip the actual memoized operation, but mitigate cascade updates after it" so if the selector runs again it should not be a problem. And the default Sweet-state behaviour (of shallow compare even the output before letting an update fire) will already stop many scenarios from triggering a re-render, like in the example of favourites on a side panel.

I think I haven't realized that the output of the selector was shallow compared. I assumed it was a reference equality with a new list breaking the memoization. Sorry about not checking the code first, It is clear in the docs (As long as the user returned by find is shallow equal to the previews one) but somehow tricked me to think o/w. This will indeed prevent many unnecessary updates, and probably would end this issue just from the beginning 🤦 .

Check this out, I was totally wrong;

On the other hand, think about a simple operation where we have id indexed objects in the state and we want to display them in specific order so that in the selector, we create a new array every time the selector gets executed. As we will be creating a new reference each time, the optimisation does not apply for this case.

Still though running all those selectors on every update may build up, but the justification for that is keeping the state small so that we won't have too many of them, which is acceptable.

We are currently doing the memoisation on a wrapping custom hook and returning useMemoed selectors, which works fine except for action usability. This is preventing any of the problems we have discussed.

I agree that sweet state should not expand its API. One thing coming to my mind though maybe exposing the equality check to give a little bit control to the consumer. The pre-packed comparison is pretty good and fits well with the intention though. Expanding the docs seems the way to go here.

Thanks for the great discussion! I have learned a lot from it.

@anacierdem
Copy link
Author

This maybe caused my initial confusion, I don't know. Thanks for fixing that too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants