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

[Enhancement] Selector Debugging Tools #279

Closed
skortchmark9 opened this issue Aug 28, 2017 · 21 comments
Closed

[Enhancement] Selector Debugging Tools #279

skortchmark9 opened this issue Aug 28, 2017 · 21 comments

Comments

@skortchmark9
Copy link

skortchmark9 commented Aug 28, 2017

Firstly, I love reselect, it's possibly my favorite part of the redux ecosystem. I love it so much in fact, that I could be accused of overusing selectors - I like to shove as much logic as possible into them, so that my teammates and I can share computed data in a performant way across our app.

Because we rely so heavily on selectors for our logic, selectors are often where our bugs crop up. Redux's debugging tools are excellent, and I'm imagining reselect could pair its debugging tools nicely along with them. A particular emphasis is "No Print Lines". I like that all the information about my app's state store is already present for me to view without a code change. If all the information about the app's computed state were available also, I would never have to change my code to figure out what was broken.

Ideas

1. Graph

The first idea is essentially the same as what was originally proposed in #71 : a visual dependency graph of selector parents and children:
image
(picture grabbed from #71 , originally from @MattSPalmer)
This would be really useful for refactoring heavily depended-on selectors, as well as debugging.

2. Click-to-select

The ability to click a selector on that graph to see what its inputs / outputs are at a given state.

I don't really like the logging solutions in #71 because they don't let you surgically inspect a particular selector without making code changes. Same issue with @ajwhite 's https://github.com/ajwhite/reselect-devtools.

Implementation

I don't think it would be too hard to make a drop-in wrapper for createSelector which populates the dependency graph:

function createSelector(...dependencies) {
  const resultFunc = dependencies.pop();
  const selector = Reselect.createSelector(...dependencies, resultFunc);
  selector.dependencies = dependencies;
  Redebug.selectorGraph[selector] = selector;
  return selector;
}

And click-to-select just needs getState() injected, either via middleware or something even more vanilla:

Redebug.getStateWith(() => store.getState());

Challenges

Naming

The one thing I am struggling with is getting the selector names. They're usually anonymous! In my createSelector wrapper, I'm keying the graph based on the resultFunc's toString(), which is going to result in pretty ugly display names. For example:

const currentUser$ = createSelector(ui$, users$, (ui, users) => users[ui.currentUser]);
=> "(ui, users) => users[ui.currentUser]"

A hackish trick that I tried was to name the selectors based on resultFunc's arguments:

const dependencyNames = getParamNames(resultFunc);
const selector = createSelector(...dependencies, resultFunc);
selector.dependencies = dependencyNames.map((name, i) => ({ name, selector: dependencies[i] }));

But even if I had a perfect argument-name-parsing regex (hard!), it could still break if users used a selector in two places but named its output differently in resultFunc, or if they used object destructuring, or if they uglified their code.

Obviously, we could add string names to the functions but that would add more overhead than I'd like for users of this lib.

[EDIT] Ok, I thought about it for a while and I don't think there's a way to get the names across every environment without writing them explicitly somewhere. I think what I'll do instead is give this lib another function (just one!) called:

Redebug.registerSelectors({selectorName: selector});

This'll be opt-in, otherwise I'll just give the selectors names based on their position in the tree.

Conclusion

  • So I'd love to hear if y'all think this is a good idea
  • Or if there's some prior work I missed that solves these problems for me
  • or if you can help me with my selector name issue.
    Thanks for reading!
@skortchmark9
Copy link
Author

Just so ya know I'm gonna make this happen - I've got a little proof of concept here and I'll post again when I've got something usable.
screen shot 2017-09-17 at 11 05 36 pm

@cfilipov
Copy link

I LOVE this idea. I could also be accused of overusing selectors. One of my app data models in particular relies on lots of data that's derived from the redux state tree (things like totals, moving averages, and lots of other stats that are derived). By having these things in memoized selectors it made the logic much simpler not having to keep all the data in sync, and perf is quite good. However, as you mentioned, having so much of the data model in selectors means I get less value from the redux dev tools because a large portion of my state is actually hidden from the tools.

It would be great if the tool kept a count of how often the selector was able to return cached data vs re-computing. Imagine if each node in your screenshot has a label such as "23/1195", meaning this selector was called 1195 times and 23 of those times required re-computing due to input change. This can help find places where people are unintentionally causing the selector to recompute or where the selector wasn't needed in the first place.

@skortchmark9
Copy link
Author

skortchmark9 commented Sep 28, 2017

Yeah that's a great idea, I'd love to know which of my selectors is doing the most work, and how effective its memoization is. I know some people like to increase the size of the cache in the memoize, so that feature would allow one to experiment with different cache sizes in order to optimize the performance.

I'm not quite sure what the best way of implementing it is. It's really nice that Reselect already keeps track of the cache hits, but considering the performance choices that have been made, it seems like the core lib shouldn't also keep track of cache misses (@markerikson thoughts?). It seems like at a minimum, we'd need to change the use of defaultMemoize to memoize here so that we could pass in a custom function to track it.

Looks like that work is being done here: #297

@markerikson
Copy link
Contributor

Not sure I have specific thoughts, other than I think this is a good idea to play around with.

@austinfox
Copy link

How's the progress on this? Anything I can help out with?

@skortchmark9
Copy link
Author

I have the client library done, but I'm new to chrome extension development so I've been dealing with hurdles there. My plan right now is to release the library first and get some feedback on it - so you could help by reviewing the code or beta testing for me! Thanks for offering!

@jordanmkoncz
Copy link

This is a great idea, I was just thinking how useful something like this would be and came to the issues to see if anyone had tried or succeeded in getting this sort of debugging set up. I agree that being able to see cache hits/misses would also be very useful.

I use https://github.com/jhen0409/react-native-debugger rather than Chrome itself for showing the React devtools, Redux devtools and standard Chrome devtools while working on React Native projects. Hopefully it will be possible to use the client library in this context once you've got it all working.

@skortchmark9
Copy link
Author

Here is the client library:
https://github.com/skortchmark9/reselect-tools

If you have any feedback whatsoever, please create an issue. This is also my first npm lib, so let me know if there are any package-maintainer things I ought to be doing but am not.

I apologize for the delay, I've been quite busy at work. I'm hoping to bang out the extension pretty soon!

@skortchmark9
Copy link
Author

Here is the chrome extension.

As is, I've found that it's useful for allowing me to identify selectors that are recomputing often and why. It's also handy for sanity checks to see what a given selector's inputs and outputs are at any given time.

There's still quite a few rough edges, and I think I'm only scratching the surface of what's possible. Some of my ideas (and yours!) are listed here

@ellbee
Copy link
Collaborator

ellbee commented Nov 22, 2017

This looks really neat. I've added a link to it in the related projects section of the readme.

@skortchmark9
Copy link
Author

skortchmark9 commented Nov 27, 2017

Hey @ellbee thanks! I actually just updated the verbiage so I'll put a PR (#302) in to reflect that the extension is released.

Speaking of which! I've now published a simple demo page that you can pop open the devtools on so that you can get a better idea of the features before instrumenting your app. As I mention in the new getting started section, it would be great if reselect itself could have the hooks for the devtools built in. @markerikson mentioned you were the main maintainer of this lib. Could we have a chat some time to discuss that possibility? What is a good way to reach you?

On another note I have actually been using the tools to make performance improvements to my app!
screen shot 2017-11-26 at 10 19 47 pm
I think I was having the same problem as #300, where I had something like:

const foo$ = (state) => state.foo;
const bar$ = createSelector(foo$, (foo) => foo.baz);
const bar2$ = createSelector(bar$, (bar) => bar + 2);

Switching to

const bar$ = (state) => state.foo.bar;
const bar2$ = createSelector(bar$, (bar) => bar + 2);

Solved a performance issue I didn't even know I had.

@alexanderchr
Copy link

Wow, this looks really cool! Definitely gonna try it out, been looking for something like this since i started using reselect.

Do you think it would be possible to measure recompute time by providing a custom memoize function? That could be really useful when looking for bottlenecks.

@skortchmark9
Copy link
Author

Hmm you mean the amount of time each selector has taken up total? Or something like what @cfilipov suggested above - % of the time it recomputed? At any rate, that work is probably blocked by #297

@jrnail23
Copy link

@skortchmark9, I think this is a great idea, and I'd really love to be able to take advantage.
Unfortunately, as you noted in your README file, the setup is pretty burdensome. Also, since we use re-reselect here and there (which also wraps createSelector), I don't quite see how I can use your tools to instrument the selectors it creates.

Has there been any movement toward getting the necessary hooks/integration points added to reselect itself?

@skortchmark9
Copy link
Author

@jrnail23 nope. There are two outstanding PRs w/green builds which would remove steps: #251 and #297, but the maintainers (@ellbee ? ) are pretty absent.

@ghost
Copy link

ghost commented Sep 26, 2018

@skortchmark9 Looks like #251 and #297 are in! Any hopes of getting this moving again? It would be amazing to have this readily available in the chrome store :D!

@skortchmark9
Copy link
Author

They are in but not yet released, so I've been holding off on doing any work. As soon as 4.0.0 is out I'll update the tools.

Also - the extension is actually already available: https://chrome.google.com/webstore/detail/reselect-devtools/cjmaipngmabglflfeepmdiffcijhjlbb

@ellbee
Copy link
Collaborator

ellbee commented Oct 1, 2018

Hi @skortchmark9, v4.0.0 has been released now.

@markerikson
Copy link
Contributor

Following up on this (very belatedly). @skortchmark9 , anything else that needs to be done here to help you?

@skortchmark9
Copy link
Author

Nah, IIRC I picked up these changes and released a new version. Closing this down.

@markerikson
Copy link
Contributor

Cool. FYI, I'm temporarily stepping in to help with some Reselect maintenance, and will be posting a v5 roadmap discussion thread "soon" (hopefully next couple days if I can find time to finish writing it). Would love to have you take a look at it once it's posted!

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

8 participants