-
Notifications
You must be signed in to change notification settings - Fork 46
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
Resolver function does not receive same arguments as the final selector function #48
Comments
Hi @enisdenjo, I guess this is a documentation phrasing issue which led to a misunderstanding. Resolver (now keySelector) function actually receives the same arguments as the final selector. Final selector is intended as the selector function returned by // "deriveFullName" is indicated as "final selector" in the docs
const deriveFullName = createCachedSelector(
getName,
getSurname,
(name, surname) => `${name} ${surname}`, // result function
)(
// resolver/keySelector function receives johnDoe as argument (like deriveFullName selector)
(personObject) => {}
);
deriveFullName(johnDoe); Resolver/keySelector function can only receive selector's arguments because the derived data is still not available on resolver evaluation time. I'd like to address this documentation issue, though. How would you update it in order to make it more straightforward from you point of view? Greetings! |
Hey @toomuchdesign, I mixed the final selector with the resolver function! I now see that you are passing additional arguments everywhere in the examples, not deriving them from a single source/selector... In this case its just a matter of not reading the README. I kinda approached the library expecting that the cacheKey function receives the same arguments as the resolver function and went with it haha. What I would change is making the README more like how reselect shows. By that I mean that in the 1st example instead of writing: ...
const selectorA = state => state.a;
const selectorB = (state, itemName) => state.items[itemName];
... you could write: ...
const itemsSelector = state => state.items;
const itemWithIdSelector = (state, props) => state.items[props.itemId];
... Maybe apply the same practices throughout other examples? Sorry about the incorrect issue! |
Hello @toomuchdesign, I can assure you that its not an issue of "not reading the docs" - I read through the readme maybe ten times, without success, because I understood it that the resolver receives same args as last defined selector as well! Then I got really angry and got the idea "what if the last version is broken?" which made me look into issues and find this one. After reading your answer it makes perfect sense (of course we need to compute cache key first, before actually calling other selectors), and I even had this thought while trying to debug my code as well (I've seen that resolver receives store state + component props and realized that selectors are not called before). I think that the examples are a bit confusing and it would help to make them more explicit (showing some real use case, not just some abstract generic "data"), and also put all related code together (selector declaration, call site and/or map state to props), instead of spliting it into multiple code blocks. I can post here my idea how it could look to be easier understandable, if you are willing to update the docs. Have a nice day! PS: I think it can be harder to understand for people who don't have English as their native language since "final" word might be interpreted ambiguously and the actual meaning of "final selector" phrase is not explicitly defined in docs. |
Hi @malyzeli, Greetings! |
Do you want to request a feature or report a bug?
I am reporting a bug.
What is the current behaviour?
createCachedSelector
resolver function does not receive same arguments as the final selector function.What is the expected behaviour?
createCachedSelector
resolver function should receive same arguments as the final selector function.Steps to Reproduce the Problem
Check the example in here: https://codesandbox.io/s/w06x9moxnk
The text was updated successfully, but these errors were encountered: