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

Version 5.0.0 proposal #401

Closed
josepot opened this issue Mar 20, 2019 · 33 comments
Closed

Version 5.0.0 proposal #401

josepot opened this issue Mar 20, 2019 · 33 comments

Comments

@josepot
Copy link

josepot commented Mar 20, 2019

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 and defaultMemoize).

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 today
  • createStructuredSelector: like the current one, but removing its second parameter
  • createKeySelector: 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.

@theKashey
Copy link

theKashey commented Apr 16, 2019

👍from a low-level API perspective, your idea is perfect. The only moment I don't quite get - .use nobody is calling(except .use itself) and how you are going to clean memory.

I've also spiked a few ways to make reselect more stable:

  • kashe - a WeakMap based memoization. It just stores result inside the argument itself.
    It's possible to have a unique pair state -> selector. For example, I would have a unique user record from a first selector, I could use as a key for WeakMap, which is just a local variable inside createSelector.
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.

  • a year ago I've written another library - memoize-state, which also could handle only one, last, result. In the same time, I wanted to have proper per-instance memoization, so I've used redux mapStateToProps factory to separately wrap mapStateToProps for every component. In the same time, I still wanted to have shared cache for non-per-instance selectors. So I've wrapped it twice - the first level is per-instance memoization, with the second level shared across instances.

  • later I've used the same idea in kashe inbox function - it "commands" internal cache to use another "weakmap" storage, "forking" realty. As a result - you might have per instance memoization(cache is bound to the "instance") from one side, and shared one from another side.

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)

  • the third approach is to utilize hooks. Not hooks, but idea behind hooks - "stack".
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:

  • cache should be stored in weakmaps
  • per instance memoization should be easy and automatic
  • API to start(createRef) or set(globals) cache dimension should be exposed
  • it shall not be bound to React

I am gonna try to spike useReselect to try the idea in a wild.

This all mean only one - keep moving forward!

@theKashey
Copy link

https://github.com/theKashey/useReselect

@josepot
Copy link
Author

josepot commented Apr 16, 2019

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.

The only moment I don't quite get - .use nobody is calling(except .use itself) and how you are going to clean memory

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:

  1. Use a custom connect HOC, rather than directly using the one provided by React-Recux, something more or less like this:
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} />;
    }
  }
}
  1. Use React-Redux-Lean instead of using React-Redux (which BTW: it has hooks already)

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.

👍from a low-level API perspective, your idea is perfect

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 createKeySelector. My coworker tried that and... voila! Everything worked amazingly well! The problem was gone and the rest of the App was fully functional.

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:

I've also spiked a few ways to make reselect more stable:

Onward, so that we can continue that conversation in there? Thanks!

@faceyspacey
Copy link

faceyspacey commented Apr 17, 2019

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.

@theKashey
Copy link

I was hopping that my idea was perfect from a high-level API perspective 😅.

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 keys automatically, or help you build right selectors, ie hight right spots to hammer.

The current one is a low-level, and, honestly, is a best fit for reselect from API look-n-feel prospective.

@ellbee
Copy link
Collaborator

ellbee commented Apr 19, 2019

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 defaultMemoize and createSelectorCreator to be escape hatches but I have no idea how widely used they are in practice. This library get 1,200,000(!) downloads a week so if we go this route we will have to think about how to make the upgrade as painless as possible.

@josepot
Copy link
Author

josepot commented Apr 19, 2019

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 defaultMemoize and createSelectorCreator to a different repo (reselect-old-internals or something like that) and then provide a codemod to make the migration to v5 as painless as possible for everyone.

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 createSelectorCreator myself 😅, so maybe I'm wrong...

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:

  • A createBranchSelector or a createSwitchSelector or something similar could be pretty cool... For those cases when the output of a selector determines which dependency should be evaluated. The same functionality can be accomplished today using createSelector and then doing that branching in the compute function. However, that implies that all the dependencies will be evaluated when there is no need for that. Providing a declarative API it's a win/win: it reduces boilerplate for the developers, it makes their code more declarative and it allows the library to better understand how things are being used, which is what makes perf improvements possible.

  • A propSelector could be very interesting too. So that Reselect can differentiate between those selectors that actually perform computations and the ones that just act like lenses on the state... IMO that could open the door to some pretty cool perf improvements, but it would take me too long to explain this, so I will leave it here 🙈

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!

@markerikson
Copy link
Contributor

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

@josepot
Copy link
Author

josepot commented Apr 19, 2019

FWIW, a quick way to get a napkin math estimate of usage is to do a code search on Github:

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).

@markerikson
Copy link
Contributor

On the flip side, do these have to be removed? Could the new stuff be added and the existing APIs be left in place?

@josepot
Copy link
Author

josepot commented Apr 19, 2019

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" createSelector just using createSelectorCreator anymore and defaultMemoize would no longer be true to its name either.

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 createSelectorCreator with the "standard" selector-creator of Reselect quite challenging.

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 😬)

@josepot
Copy link
Author

josepot commented Apr 19, 2019

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 defaultMemoize function or its enhancer, because the way that Reselect chooses to memoize things should be hidden from the API (it's an implementational detail).

@mpenney99
Copy link

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:

const getWidget = createKeyedSelector((state, id) => id, (id) => {
		return createSelector(getDashboard, (d) => d.widgets[id2]);
	));

getWidget(state, id);

multiple keys:

 const getWidget = createKeyedSelector(
		(s, { id1 }) => id1,
		(s, { id2 }) => id2,
		(id1, id2) => {
			return createSelector((s) => s.dashboards[id1], (d) => d.widgets[id2]);
		));
	
getWidget(state, { id1, id2 });

@josepot
Copy link
Author

josepot commented Apr 19, 2019

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.

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 createStructuredSelector?

@mpenney99
Copy link

mpenney99 commented Apr 19, 2019

Do you think that this proposition damages Reselect's simplicity

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!

Also, how is the alternative that you are suggesting any better than using factory-selectors or re-reselect?

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:

const getWidgetProps = createKeyedSelector((state, id) => id, (id) => {
	return createStructuredSelector({
		propA: getWidgetPropertyA,
		propB: getWidgetPropertyB,
		propC: getWidgetPropertyC
	})
));

const { propA, propB, propC } = getWidgetProps(state, id);

How composable would those selectors be? What would happen if I tried
to use one of those selectors inside a createStructuredSelector?

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:

const { getWidget, getComputed } = createKeyedSelector((state, id) => id, (id) => {
		const getWidget = createSelector(getDashboard, (d) => d.widgets[id]);
		return {
			getWidget,
			getComputed: createSelector(getWidget, computeSomethingExpensive)
		};
	));

Edit:
All in all, I think this all depends on the direction we want to go with this library. Would a small addition like this cover most of the use cases? I have to say though, I really like the "transparency" aspect of your proposal, and your article explains the issue very well too. So I'll leave this for "smarter" people than me to argue over! :)

@josepot
Copy link
Author

josepot commented Apr 19, 2019

Hi @mANDROID99 !

I'm very sorry if my initial response came across as snappy. Sorry, that was not my intention.

So I'll leave this for "smarter" people than me to argue over!

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 createKeyedSelector. Otherwise, I get a new recomputation and that could end-up triggering pointless re-renders (the same problem that re-reselect has). My goal is to create an API that hides those implementational details. So, that we can let the library take care of those nasty things for us 🙂

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.

@markerikson
Copy link
Contributor

(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.)

@josepot
Copy link
Author

josepot commented Apr 20, 2019

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!

@josepot
Copy link
Author

josepot commented Apr 21, 2019

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 createKeySelector accomplishes different things:

  • It declares that what this "selector" returns is an ID that did not come from the state (at least, not directly). That's why the base function that is being enhanced by createKeySelector doesn't receive the redux-state. A naif implementation of createKeySelector would be like:
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.

  • This also lets the library know that the result of that selector is an ID. However, that doesn't exactly mean that the result of that will end up becoming the key of the cache. For instance, consider this:
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 getJoinedUsers, a combination of both results (getIdA and getIdB) will end up being used as the key of the cache. I know, I know... It's almost the same. But the pedantic point that I'm trying to make is that we are not giving a means to allow specifying a cache key. We are providing an enhancer for creating a type of selectors that are different from the rest because they return IDs that didn't come directly from the state. How the library decides to take advantage of that information is another story 🙂

EDIT:

I just realized that the name createKeySelector doesn't really help at separating the API from how things work internally 😬

I think that another name would be much better suited... I think that something along the lines of getExternalId would be a much better option. 🤔

I'm about to release a new version of Redux-Views. I will definitely take this into account.


(*1) When I say that:

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

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.

@voliva
Copy link

voliva commented Apr 21, 2019

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 master it's how I initially had it and has the performance issue (try typing really fast on the form or slow down your cpu in devtools performance tab) - In there you can check how every single keystroke results in all of the components re-rendering again. And then I have one branch for each possible solution:

  • make-reselect: Solution proposed by the current API, which uses selector factories.
  • re-reselect: Solution by using re-reselect.
  • redux-views: Solution by using the proposed API, currently available as unstable on redux-views.

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.

@ellbee
Copy link
Collaborator

ellbee commented May 17, 2019

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!

@josepot
Copy link
Author

josepot commented May 17, 2019

Thanks a lot @ellbee! And no worries, I have 2 small kids. I get it 🙂

@markerikson
Copy link
Contributor

markerikson commented Aug 9, 2019

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.

@nickserv
Copy link
Contributor

nickserv commented Aug 9, 2019

@markerikson
Copy link
Contributor

markerikson commented Aug 9, 2019

Given that the types are here in the lib repo, I'd like to see someone put up a proof-of-concept PR that:

  • Drops the multi-arg signature for createSelector
  • Tries to simplify the typings with both that change and whatever new spread-operator or whatever capabilities there are in the latest versions of TS

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.)

@theKashey
Copy link

From TS3.5 prospective new array tuples might be easier typable. Not promising I would do it, but I do have a few libraries with (modern TS) types "compatible" with reselect signature, so I would try.

@ellbee
Copy link
Collaborator

ellbee commented Aug 9, 2019

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.

@markerikson
Copy link
Contributor

@ellbee : sure. Out of curiosity, any chance you could put the experimental branch up just as a point of comparison?

@ellbee
Copy link
Collaborator

ellbee commented Aug 9, 2019

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! 😅

@ellbee
Copy link
Collaborator

ellbee commented Aug 9, 2019

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.

@josepot
Copy link
Author

josepot commented Sep 28, 2019

Hi @ellbee and @markerikson I'm seriously considering opening a PR for Reselect v5. For more info see this: josepot/redux-views#16

Thanks!

@markerikson
Copy link
Contributor

@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!

@sneakyfildy
Copy link

hi, what's up with having more than 12 selectors?

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

No branches or pull requests

9 participants