-
Notifications
You must be signed in to change notification settings - Fork 4
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
Other approaches to make Reselect more stable by @theKashey #12
Comments
I, personally, would prefer your solution over my ones. I like it, and I could use it. But I could not say the same about my teammates and especially about my wife - they are having problem with proper classical reselect usage, and additional API would not be welcomed at all :) It is possible to create perfect selectors with our library, and with hooks it just shines. It's possible to have per-key memoization shared among different instances. There is one way, which I didn't try yes - a solution from testing or declaration prospective. Right now you are adding some |
Hi @theKashey ! Sorry that it took me a couple of days to get back to you. First off, I'm not going to try to convince you to use my solution or anything like that. I'm well aware that there are other issues that have to be improved when it comes to deriving data from a Redux state. So, you should use whatever makes you more productive and comfortable, for sure. I think that the API change that I'm suggesting will not only solve the problem with instance-selectors but it will also open the doors to further API improvements. The only thing that I would like to ask you is to please point out to the issues that the new API doesn't solve for you. All that being said and before I give you feedback on the alternatives that you have worked on. I think that it's important that I explain those things that for me are paramount when it comes to designing good APIs (full-disclosure: my opinions are heavily inspired by this talk of Paul Phillips): I'm of the opinion that perf optimizations are implementational details, and as such we should try to hide them as much as we can from our APIs. In the case of Reselect (or any library for generating derived data, for that matter): memoization is a means for improving performance, therefore it should be considered an implementation detail. I also think that it is good and necessary for a library to establish a set "rules"/"constraints" about how that library is meant to be used. For instance, for Reselect those constraints are that Reselect will only work correctly if the state is immutable and the compute-functions are referentially transparent. Reselect does not set these constraints arbitrarily, it sets them because there are lots of devs that have chosen to work within a paradigm that values the advantages of those "constraints". Finally, a good API helps both the developer and the library. I kinda explain this already in the latest comment that I've posted on the Reselect's issue:
Now that I have already bored you with my "good API principles" 😅 I think that you will understand why I'm not a fan of your approaches: they all put memoization at the forefront, and they force devs to have to think about how a given transformation will be used, rather than just focusing on the transformation itself. I think that if we find a good API for building "reactive transformations" and we hide its implementational details, then the performance will come naturally. So far, the solution that I'm proposing with Redux-Views already has:
(To be continued... Sorry, I still have to answer to Mark Ericson, plus I have to finish the answer to James Gillmore.) I realize that I have not answered this to you yet:
I will very soon, I promise. Because even if it's an impl detail, I'm pretty proud of my solution 🙂 |
That is true, but not with Redux, where any change to a state could update the whole everything.
Sounds like MobX :) Unfortunately mobx computed fields are much less "implementation details" than reselect. |
I'm creating this issue in order to separate the following conversation initiated by @theKashey from the main topic of that issue (the proposed v5 of Reselect).
The text was updated successfully, but these errors were encountered: