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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thank you #5

Closed
trungdq88 opened this issue Mar 23, 2018 · 4 comments
Closed

Thank you #5

trungdq88 opened this issue Mar 23, 2018 · 4 comments

Comments

@trungdq88
Copy link

...for creating this awesome project. I would buy you a beer 馃憤

@skortchmark9
Copy link
Owner

Awwww how nice! Are you using it in your app? Did you solve any problems with it?

@trungdq88
Copy link
Author

trungdq88 commented Mar 24, 2018

I was always thinking about building something like this (maybe around a year ago) but never putting real effort to do it. Yesterday I decided to give another try, and somehow find this, incredible.

image
(I screenshot this to show my excitement with co-workers last night)

I integrated and it immediately brings benefits to my last project by saving my time debugging selectors input/output. My next project will definitely use this, I can foresee this saves me tons of time debugging and console.loging.

My most favorite features:

  • Clicking to a node highlights it dependences and dependents.
  • Work with "time travel" feature in Redux Devtool (using slider)!
  • Showing recompute count for each selectors.

If there is anything I could help, here are some feedbacks:

  1. Exception handling: Sometimes it shows "Could not load selector graph", I figure out it is because __RESELECT_TOOLS__.selectorGraph() was throwing exceptions (null handling issue in selector logic), after fixing it all, I could get the graph rendered nicely. To improve this, I we should have a try/catch in this line https://github.com/skortchmark9/reselect-tools/blob/master/src/index.js#L59 and then in the output panel, we can show the exception detail.
  2. Selector name conflict: We should somehow supports namespace to avoid naming conflict (many selectors could have a same name in large apps). I was doing this:
const withNamespace = (namespace, selectors) => {
  const keys = Object.keys(selectors);
  const newObj = {};
  keys.forEach(key => {
    newObj[namespace + '/' + key] = selectors[key];
  });
  return newObj;
};

ReselectTools.registerSelectors({
  ...withNamespace('auth', selectors1),
  ...withNamespace('tnx', selectors2),
});

We should consider adding a built-in API to register selectors with namespace.

  1. We could arrange the graph nicer, need some spacing for the selector names.
  2. Could be nicer if we can somehow resolve the selector names without registerSelectors for each selectors files (less step) - I don't know how yet but I can "feel" we can do that.

I could send PR if you have a contribution guide.

@skortchmark9
Copy link
Owner

Wow so helpful! Thanks dude! These are all really great ideas. I'd take a PR for basically any of them - I don't have a contribution guide but I'd be down to do a code review. I'd just ask that:

  • you keep the test coverage about where it is
  • update the README to document any new features/api you add.

I thought quite a bit about 4.) and I couldn't think of a good way to do it that would work in a minified / uglified build. If you're not minifying, then you can get pretty far doing introspection on the function name, or on the names of the arguments of a descendant selector.

@skortchmark9
Copy link
Owner

Of these four, I think the most necessary improvement is the exception handling in selectorGraph(). I've definitely seen selector exceptions be swallowed before in unpleasant ways and it would be great to expose them better. I'll break off a new issue for that, but I'm going to close this one down.

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

2 participants