-
Notifications
You must be signed in to change notification settings - Fork 54
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
Comments
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. 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 |
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 This is an initial attempt (not tested):
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 |
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 Because it's not just about adding new code. There are subtleties that I'd like to get right before extending the library. Should sweet-state instead provide a custom version of |
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 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;
|
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 Basically I'm not changing the current API, but I'm improving 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!). This means that 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 cc @theKashey |
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 🤔 |
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, |
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 |
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 (
Anything else, like weakmap based cache, babel based magic and so on might not that great, and may require API changes as well.
The new selectors are based on the same idea, but please consider saving the result to some "global" cache as well. |
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 :) |
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. But if you have some ideas, shoot! This is the right time as nothing has been finalised yet |
I'm not sure I get the problem with nested selectors on redux. It is possible to create all the selectors inside
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;
|
By creating selectors in There is a way to make this moment better (but not solve):
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. |
@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? |
Thanks for the input guys.
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 I start to think that the complications arise when we try to combine state selection with derived data computations. From Sweet-state perspective, what could be added to simplify the support of such patterns is allowing |
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 ( Check this out, I was totally wrong;
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 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. |
This maybe caused my initial confusion, I don't know. Thanks for fixing that too! |
When creating a hook, it is possible to provide a selector. Consider this contrived example which will just select a property on the store;
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 ofvalue1Selector
orvalue2Selector
(which might also be memoized) changes;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.
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.The text was updated successfully, but these errors were encountered: