-
Notifications
You must be signed in to change notification settings - Fork 674
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
Version 5.0.0 proposal #401
Comments
👍from a low-level API perspective, your idea is perfect. The only moment I don't quite get - I've also spiked a few ways to make reselect more stable:
const getUserSummary = createSelector(
getUser,
....
doSomethingWithItAll
)
// ---- something like
WeakMaps[getUserSummary].set(getUser(state, id), result); // This approach hovewer does not play well with simple selectors like const getUsersInGroup = (users, group) => users.filter(user => user.group === group) as long as you cannot use group(string) as a key for a WeakMap, and still able to hold only one result. But it might be not a problem.
const getUsersInGroup = (users, group) => users.filter(user => user.group === group)
const memoizedgetUsersInGroup = kashe(getUsersInGroup))
const mapStateToProps = () => {
const perInstance = kashe(memoizedgetUsersInGroup);
return (state,props) => ({ user: perInstance(state, props.group) });
}; Unfortunately this still requires some engeneering mindset. Fortunately could be done automatically from redux side (needs some injections)
const createSelector = () => {
return () => {
if(!GLOBAL_RESELECT_CACHE) {
// first selectors reads a shared variable from a fiber.
GLOBAL_RESELECT_CACHE = useRef();
}
try{
pushSelector();
.... everything as usual, all cache is in GLOBAL_RESELECT_CACHE
} finally {
if(!popSelector()) {
// redux could read this variable, and inject it before running mapStateToProps by itself to mitigate `useRef` usage.
LAST_GLOBAL_RESELECT_CACHE = GLOBAL_RESELECT_CACHE;
// last selector cleans state
GLOBAL_RESELECT_CACHE = null;
}
}
}
} Maintaing "stack" manually would allow to escape from hooks rules, and from hooks itself. Just store result in two places - and call it a day. The solution could be memory efficient, with automatic per-instance cache cleanup with instance end of life. Still could use "standard" reselect underneath to share shareable data. Drawback - you might call the same selector with different props even within one component, and this approach would not help. Overall:
I am gonna try to spike
|
Hi @theKashey , Thanks a lot for your feedback. Before I share my opinions on your attempts to "make reselect more stable", let me explain a few things about what I'm proposing that I have not explained very well. Sorry about that.
There is this section inside the Redux-Views README that talks about that. Basically, what it says is that -as of today- if you want to have "automatic" cache invalidation, you have 2 options:
const customConnect = (selector, ...rest) => {
const { keySelector, use } = selector || {}
return Base => {
const Component = connect(selector, ...rest)(Base);
return props => {
const key = useMemo(
() => keySelector ? keySelector(null, props) : undefined,
[keySelector, props]
);
useEffect(() => use && use(key), [use, key])
return <Component {...props} />;
}
}
}
I'm currently working on a much better version of React-Redux-Lean, which should improve its performance dramatically... We will see. However, if Reselect decided to accept my version 5 proposal, we should probably convince React-Redux to account for "usable" selectors, and I can see how that could be problematic/polemic. IMO React-Redux already set a similar precedent by accepting factory-selectors... But I guess that it doesn't really compare.
I was hopping that my idea was perfect from a high-level API perspective 😅. Let me explain why I say this using a real-life example: The other day at work one of my co-workers (@voliva) was having problems with a React-Redux App because a form was super sluggish. We debugged that together and we found that the issue was that all the instances of a list component were being re-rendered every time that the user pressed a key on a form-field (and their selectors were being re-computed for "no reason"). You can already imagine what the problem was, right? They hadn't used factory-selectors for creating the "shared-selectors" of the items of that list. For me that was the perfect chance to ask for a favour: Could you please remove reselect, add redux-views and use webpack to force Reselect to resolve to Redux-Views? Also, please change the definition of one selector so that it uses I asked @voliva not to use that in production (yet). However, it made me happy to see that the API that I'm proposing for version 5 is compatible with the way that most devs use Reselect, and that at the same time it solves the "shared-selectors" problem. I was going to let you know what my thoughts are on your different spikes of how "to make reselect more stable", but then I realised that that is a separate conversation and that it is a bit out of topic in here. In this issue I would like to discuss whether this new API that I'm proposing makes sense or not, and if there are any other draw-backs apart from the ones that I'm already pointing out. Please, do not get me wrong, I want to share what my thoughts are on your spikes, for sure. However, since you are not proposing any of those as a possible next version major version of Reselect, I think that it would be better to have that conversation in a different issue 🙂. That is why I have created the following issue in Redux-Views, so that we can continue that conversation there. Could you please copy-paste in that issue, from the line:
Onward, so that we can continue that conversation in there? Thanks! |
I wouldn’t be so concerned about contributing back to reselect. I would be happy to use your product. If you are the most motivated in this space as of 2019, build it under your name and collect credit which will help you get jobs and clients, which in turn will reinforce your motivation to maintain and evolve it. Having successful repos under your name can make you hundreds of thousands of dollars and change your life. My recommendation is to not martyr yourself. The sentiment is nice, but isn’t necessary beyond getting the community to look at your solution. Fork React-Redux if you have to (which I see you have). Fork Redux itself. Preserving the old for so long has just resulted in decay. Redux is in great need of a makeover; it’s inevitable. There’s so many places we never took these tools—mainly because people get the wrong idea that there’s nothing left to do and it’s “done.” What you are working on is a key piece. Be bold and confident in what you intuit to be the way forward. If, on the other hand, you’re not motivated to carry the flag for very long and just wanna pass off your discoveries, that’s another story. But don’t be afraid to replace the status quo—that’s what software is about and what is currently lacking in the stagnant React landscape. With hooks it might not be clear, but the pace of innovation has greatly slowed in the space compared to a few years ago...And innovation trumps fatigue, to say the least. React, with hooks and suspense, is making an attempt to replace Redux. There will need to be a Redux renaissance for it to survive. And I think it should survive because there are things we can’t do with hooks/suspense, and more importantly latent capabilities that have yet to evolve in the Redux path. What you are working on is a key pillar in the Redux renaissance. Ps. I just read your medium article. It was excellent by the way. Write more. A lot more and post them everywhere if you don’t want to be a tree that falls in the forrest that nobody hears. The core of what your doing here revolves around promotion. From experience, don’t expect the powers that be to give a shit or help you. That’s clear by how few likes you got on your amazing launch article on medium from a month ago. You deserve a lot more. That’s a sign of how shitty the React game is and how little influencers ultimately know. Don’t depend on them. You gotta do your own promotion, and an endless amount of it. The developers that know the most work on real projects, often ones they own or are technical leads in, and have little time for the misleading echo chamber of react tweets and articles. Next suggestion: build fuller tools worthy of your time promoting it. Your fork of React-Redux is an example of this, whereas your fork of reselect isn’t. In other words, build a tool big enough that it’s a primary pillar to how people build their apps, rather than a hidden implementation detail. Something with a significant api surface and many ways to use it will bring you tons of paying clients in need of your wisdom. There’s disproportionate gains for open source developers that build a comprehensive system vs those that make minor fixes. Your work is actually pretty far along the spectrum toward being a contender people need to take seriously. You are doing real innovation. Unfortunately obvious innovation that needed to be done long ago. But thankfully you are finally doing it...I make those last comments just to indicate that the ecosystem is totally (and always) up for grabs for those that have the time and vision to enhance it. |
And then please let me retell your story in other words: He walks into the factory, takes a look at the Big Machine, grabs a sledge hammer, and whacks the machine once whereupon the machine starts right up. Graybeard leaves and the company is making money again. The next day Manager receives a bill from Graybeard for $5,000. Hammer: $5. Knowing where to hit the machine with hammer: $4995 old story That's why I called it a low level API - it's a hammer. You worth the rest 4995$. The high-level API should or handle The current one is a low-level, and, honestly, is a best fit for |
Hey @josepot, thanks for this. I think it is worth serious consideration. Sharing selectors is definitely an area where Reselect doesn't push the user to "fall into the pit of success". I consider |
Hey @ellbee, thanks for considering this proposal! I totally agree. If Reselect decided to go down this road, then we should act with caution. I think that a sensible approach would be to move I think that most Reselect users don't use those 2 functions, so I hope that most devs won't need to use that codemod. Although, I have used Also, I think that it would be very interesting to know how ppl have been using those 2 functions... Because IMO hiding those 2 functions would open the door to further improvements on Reselect's API. For instance:
Anyways, I'm getting ahead of myself. However, I do think that moving those 2 functions outside of Reselect and asking feedback to the users that have been leveraging them could lead to further improvements on Reselect's API, which would also make Redux (and others) apps even more performant. cc @faceyspacey and @theKashey sorry that I responded to @ellbee before I responded to you. I have been pretty sick these last 2 days and today I'm starting to feel a bit better. I will respond to you before EOD today, for sure! |
FWIW, a quick way to get a napkin math estimate of usage is to do a code search on Github: https://github.com/search?l=JavaScript&q=reselect+defaultmemoize&type=Code |
Wow, thanks @markerikson! That is a great idea. Let's # of usages: GitHub search results of reselect + defaultMemoize: ~800 GitHub search results of reselect + createSelectorCreator: ~750 GitHub search results of reselect + createSelector: ~60,000 So, according to that "napkin math", it seems that ~98% of Reselect users are not using those 2 low-level API functions. Let's keep in mind that what I'm proposing is a major version update. So, I think that it should be "ok" to remove those 2 from Reselect. Especially if we keep them available in another repo and we provide some migration notes and tools (i.e codemod). |
On the flip side, do these have to be removed? Could the new stuff be added and the existing APIs be left in place? |
While that is technically possible, I don't think that's a good idea. Because it wouldn't be possible to build the "standard" Also, if in the future we wanted to further improve Reselect's API, that kind of enhancer would be a limiting factor... Lastly, it would make the composition of the selectors created with a custom I mean, if what you are suggesting is to create an intermediate minor version update on version 4 where we still keep those ones, before v5 is released... I honestly think that would be unnecessary, but I guess that would be reasonable. However, if we go for a major version update, then I think that the right thing to do would be to hide those 2 from the public API. (I realize that I keep using the first person plural and I'm not part of this org, I hope that you don't find that annoying 😬) |
Not to mention the fact that the current implementation of Reselect is basically those 2... Since the new API can't be implemented using those 2 functions, that would mean approximately doubling the parsing size of Reselect. So, having a tone of unused code. But I would like to insist on the importance of the main reason, which is that perf optimizations are implementational details, and they should be hidden from the API. We should not expose a |
Just to offer another point of view to the discussion! For me, one of the best thing about reselect is its simplicity. I think this proposition is a nice idea, but maybe it belongs more as a separate library. That said though, I agree that caching by key is a really common problem that could be solved in reselect itself. As an alternative, how about something along the lines of:
multiple keys:
|
Do you think that this proposition damages Reselect's simplicity? I'm basically proposing to remove the complex parts of the API and keep the simple ones so that we can improve its performance and maintainability (no need for factory-selectors). I'm a bit confused... Also, how is the alternative that you are suggesting any better than using factory-selectors or re-reselect? How composable would those selectors be? What would happen if I tried to use one of those selectors inside a |
I just mean I think it hides a lot of magic behind the scenes. Like I said, I think it's a neat API - it just feels like there could be an easier solution that doesn't require any internal changes. Removing access to "complex" internals parts to enable further evolution does sound like a good idea though, totally agree there!
factory-selectors cannot be shared between multiple (React) components. re-reselect doesn't give any easy way to control cache-value creation - I think you can do it by providing a custom selectorCreator, but it's a bit ugly. For example, I don't think it would be easy to do something like this in re-reselect:
It's just a selector function, so composition would work just the same as normal; so I'm not sure I quite understand what you mean there. Although it's true that a downstream selector wouldn't share the same cache key, so you could get some unnecessary computations. That said, you could quite easily compose selectors for same key within the closure, (e.g. with a second form returning an object), like:
Edit: |
Hi @mANDROID99 ! I'm very sorry if my initial response came across as snappy. Sorry, that was not my intention.
I just want to say that I don't think that I'm smarter than you or that what you were proposing was not smart. Actually, what you proposed is along the lines (perhaps even better) of what many other smart people have proposed before... And it is not a bad solution! However, IMO it has the same inherent issue that the other solutions have, which is that it forces devs to have to think about how a given transformation will be used, rather than just focusing on the transformation itself. And then, every time that I want to compose a keyed-selector, I have to do it through It's normal that you feel hesitant about what I'm proposing. If I understood you correctly, you like to understand how things work behind the scenes. And with the current API it's easy for you to understand what's happening because it's very straight-forward. I guess that what makes you feel uneasy about what I'm suggesting is that you feel like there is too much magic happening behind the scenes, and you are not sure about how you feel about that. I think that I understand where you are coming from. However, perhaps you would be willing to pay the price of not being that familiar with that "magic" if we presented you with a good API, a comprehensive set of tests and some critical perf thresholds that the new implementation should be able to hit. Let me explain something that happened to me pretty recently: when I was building Redux-Views I knew that I was going to need to build my own React-Redux connector. In my head, I was pretty confident that I knew how React-Redux was implemented, so I didn't even bother to look at its implementation. I just grabbed its tests and I did my own implementation using hooks, I made sure that the tests passed and I was done with that... Then later on, when version 7 of React-Redux was released a bug appeared, and I thought that I could learn a few things by looking at that bug and trying to help with its fix. Well, when I saw the way that they had implemented version 7 I was like 😮! That was a masterpiece! It was so smart! I was trying to do something pretty similar on my own, but I was taking a slightly different approach... The point is that by looking at the tests, you would have never guessed its implementation, and intuitively I think that almost no one would think that it has been implemented like that. Summarizing a lot, I thought that every time that a dispatch takes place, then every "connected container" receives the latest redux state through context and after evaluating the selector, the actions, etc it decides whether the base-component should be updated or not... Intuitively, that's what you would expect right? Well, I will just tell you that's not what's happening... there is a tree of subscriptions that are being used in a super-smart way to prevent unnecessary updates even at the container level, which is an amazing perf-optimization. I bet you that 99% of developers that use react-redux have no idea that's what's happening... and that is a good thing. I think that we should try to accomplish the same with Reselect: hide the implementational details in favor of a better API. It is a difficult thing to do once you have got many developers who are already familiar with how things work behind the scenes, but in the long run, I think that will be a good thing. Once again, I'm sorry if I was snappy with you in my first message. Also, I would like to encourage you to open a different issue making your own proposal for v5, and perhaps even create your own POC and play with it so that we can get a better sense of how your API would work in some real-life examples. I'm saying this because I honestly believe that would be the better way to compare both proposals and because I think that we could both learn from each other. |
(fwiw, "every connected container receives the state through context" is how React-Redux v6 worked - we just had to drop that because of perf issues.) |
Hi @faceyspacey ! Sorry that it has taken me so long. I finally found the time to answer your comment. I did it in this issue of Redux-Views because I want to keep this issue on the topic (the v5 proposal). Thanks again for your kind words and support! |
I want to apologize because I think that I haven't explained very well what (at least for me) the main goal of this major version upgrade should be. To be clear, to me the main goal is not to solve the problem with "shared selectors". That could be accomplished in many different ways, without the need for a major upgrade. The real goal is to hide the implementation details from the API. I think that memoization, being a technique for improving performance, should be considered an implementation detail and it should not be exposed in the API. In fact, moving forward, I think that the less the consumers of the API know about the way that the library is handling things internally (and that includes memoization), the better (*1). I think that when explaining the API we should really try not to do that based on how things work internally. That's why I made this pedantic comment on Twitter Sorry @markerikson about that! I understand that the limit of characters of Twitter makes it challenging to explain things properly. The thing is that
const createKeySelector = baseFn => (state, ...rest) => baseFn(...rest) So, this in a way also helps the developer. Because, by using that enhancer they don't have to worry about ignoring a superfluous first argument (the state)... Because, for this kind of selectors, it is not needed.
import { prop } from 'ramda'
const getUsers = prop('users')
const getIdA = createKeySelector(prop('idA'))
const getIdB = createKeySelector(prop('idB'))
const getUserA = createSelector([getIdA, getUsers], prop)
const getUserB = createSelector([getIdB, getUsers], prop)
const getJoinedUsers = createSelector(
[getUserA, getUserB],
(userA, userB) => // Some expensive operation in order to join 2 users
) In the case of EDIT:I just realized that the name I think that another name would be much better suited... I think that something along the lines of I'm about to release a new version of Redux-Views. I will definitely take this into account. (*1) When I say that:
I'm not saying that we shouldn't explain how the internals work on blog-posts for curious developers, or on the inner READMEs of the source code, etc... I mean, it's not like the internals should become secretive. It's that we should try to separate them as much as we can from the API. |
I think this proposal is really good - Through a declarative API we get great performance boons when compared to the current stable API. I wanted to chime in with a real-life example that recently happened in a project of mine - In this project I've been using reselect since the beginning, but I made a false assumption: I thought that the only performance benefit of using reselect was to avoid recomputing derived data when its sources haven't changed. So I didn't think too much about instance selectors, because, although I knew that they would recompute for each instance, the compute functions were simple enough that I didn't think it would affect performance that much (you know, it seemed like micro-performance, I'd deal with that later on). That was until I hooked a form into the store, which meant that every single keystroke produced an action that changed the form's state, in a page where I also had a virtualized list with some complex components. It was then when I realised that caching is not only important to avoid recomputing derived data, but also to avoid react-redux re-rendeing the whole instance container, as the references to that data change. I've prepared this repo in case you want to play around with this case. It's a simplified version of what happened in my project, but it's very similar. On branch
I also leave links into codesandbox if you just want to try the performance issue: If these changes were already in reselect, all I'd need to do to fix this case would be changing just one line. And that line it even become more declarative. That's just awesome. I'm really looking forward to this! I must say that I'm not completely external to redux-views. @josepot is a coworker, and helped me understand the problem I had and the possible solutions, and in turn I gave him some feedback on redux-views. As I'm quite a typescript fan, I also contributed to redux-views by adding TS definitions. |
Hey @josepot, just to let you know (and others watching this issue) that I'm going to find some time to look at this over the weekend and hopefully formulate a plan of how we might proceed. My apologies for not getting to it sooner, I have a young son and we just moved house so my time has been very limited recently! |
Thanks a lot @ellbee! And no worries, I have 2 small kids. I get it 🙂 |
Bumping this thread a bit and adding an additional thought: I know that @nickmccurdy has pointed out that much of the complexity for the Reselect TS typings comes from the current function signature. Both an array and individual function arguments are accepted, and the "output selector" comes last. I would be in favor of potentially changing the function signature in v5 to simplify all that and make the typings easier to deal with. Specifically, it seems like dropping the multi-arg option would be a net benefit. |
Linking some related discussions about problems typing the current API: |
Given that the types are here in the lib repo, I'd like to see someone put up a proof-of-concept PR that:
I realize this is somewhat offtopic for the original proposal from @josepot . But, I think both sets of changes are fair game for a potential v5.0, and this issue is already open as a discussion point. (also, per https://twitter.com/JamesRNail/status/1159668641765502976 , a codemod to change the multi-arg form into the array form seems like a very doable thing to help folks migrate.) |
From TS3.5 prospective new array |
I have been experimenting in the background (very slowly unfortunately) with changing the API to be more redux-views like, written in TypeScript and using Jest to test (which removes the dozen Babel packages, coveralls, nyc, mocha and chai dependencies) and came to a similar conclusion that either dropping the multi-arg signature or moving the combining selector to be the first argument (and then also maybe dropping the multi-arg signature?) would probably be a good idea if we are going to make a big backwards-incompatible release. As I have so little time these days I echo Mark's request for a proof-of-concept PR, it would be super helpful. |
@ellbee : sure. Out of curiosity, any chance you could put the experimental branch up just as a point of comparison? |
Yep, I don't have access to it right now but will try to push a branch over the weekend. Not sure how useful it will be as it was basically me trying to type reselect/redux-view functions in an increasingly desperate and convoluted manner! 😅 |
I’ve just had a look at where I got to and it is in better shape than I remember. I’d gone down the route of changing the API and putting the result selector first and using infer and some other tricks to get the return types of the input functions. I’ll tidy it up and push to a branch over the weekend. |
Hi @ellbee and @markerikson I'm seriously considering opening a PR for Reselect v5. For more info see this: josepot/redux-views#16 Thanks! |
@josepot hey, fyi, I'm planning to file a discussion topic on plans for Reselect v5 in the next couple days. There's several factors I want to plan out, including the changes you've proposed here. Would appreciate your feedback in that thread once it's up! |
hi, what's up with having more than 12 selectors? |
As much as I love Reselect, I hold the crazy idea that Reselect creates a leaky abstraction by exposing its 2 lowest level API functions (
createSelectorCreator
anddefaultMemoize
).I think that if it didn't do that, then it would be possible to solve the problem of "shared-selectors" in a way that it would be almost transparent for the developer. I discuss that idea at length in this post. Also, I have created this library just to make sure that what I'm about to suggest is not totally crazy.
The API that I would like to suggest for version 5 would be something like this:
createSelector
: same signature as todaycreateStructuredSelector
: like the current one, but removing its second parametercreateKeySelector
: an enhancer that lets the library know that the selector that is being enhanced returns an special type of result: the identifier of the instance(s) that consume the selector.With that API it would be possible to solve the problem of shared selectors, just like redux-views does.
Just in case that it is not obvious: I have not created Redux-Views to "compete" with Reselect. I did it just to show that it is possible to implement the API that I'm suggesting. If Reselect decides to go down this road (or a similar one) I would love to help and I would immediately deprecate Redux-Views, of course.
The text was updated successfully, but these errors were encountered: