skortchmark9 / reselect-tools

Debugging Tools for Reselect
MIT License
427 stars 24 forks source link

Support for re-reselect #21

Open bdurrer opened 5 years ago

bdurrer commented 5 years ago

I am a bit suprised to not find anything on google or the issue log here, so maybe I am just doing it wrong :)

We have a lot of re-reselect selectors, but as soon as I add them to registerSelectors, I get log-spam from re-reselect, because the re-selector's keySelector is called without the mandatory cache key parameter.

index.js:334 [re-reselect] Invalid cache key "undefined" has been returned by keySelector function.

I guess the tools would need some way to give the keySelector the correct parameters, eg a helper that you could provide or by inspecting the cached selectors. I managed to get a partial view on the selector by fetching one of the cached selectors directly:

const cachedSelector = selector.getMatchingSelector(null /* this normally the state */, 'default' /* cache key param */);
reselectTools.registerSelectors({ cachedSelector });
skortchmark9 commented 5 years ago

Hey @bdurrer - I don't use re-reselect in my own apps, so I never thought about this use case. However, if you can come up with a way to get it to work I'd be happy to take a PR

parkerault commented 4 years ago

Is this just an issue with re-reselect support, or is it an issue with any multi-arity selector? In src/index.js#checkSelector every selector is executed with the result of stateGetter, and off the top of my head I can't think of any easy way to provide a default ...rest parameter for each selector. I always use a second props object parameter, which I think is a pretty common pattern (any other parameter type can't be composed), so you could possibly have an optional second parameter for registerSelectors with a default props object. However, you would need to provide it with a key and value for every selector in the tree, which would be enormous for any non-trivial application. You could alternatively require an additional defaultProps value for each multi-parameter function, but this just splits up the same amount of work. I don't see any reasonable way to implement this feature that wouldn't require an extensive integration effort.

So in summary:

skortchmark9 commented 4 years ago

Thanks for the info! You're correct that it's probably more a consequence of the second argument - I've never had to use that myself since I tend to think of selectors more as a mechanism for holding whole-app computed state.

bdurrer commented 4 years ago

Maybe displaying all cached re-reselectors is not practicable anyway. I guess it could also have a negative impact on performance. At least in our setup, there are only a handful of possible cach entries (each app tab has it's own selector cache entry) and it would already be nice to view one or two variants of each re-reselectors.

How about having a second parameter on registerSelectors, which allows to provides a function for each selector. It's job would be to calculate the arguments for one selector, similar to what getStateWith does for the state.

The function would simply return the required input for one of the cached re-reselectors. One could easily make that function dynamically switch caches based on some extern data, e.g. a global cache-key variable, so you can browse through the selectors based on an external controller.

e.g. something like


function selectorArgs(state, selectorName) {
  if (reasons) {
    return ['param1', 'param2']
  } else {
    return ['param3', 'param4']
  }
}

registerSelectors({ mySelector }, { mySelector: selectorArgs })

would then call the selector like this

const state = _getState()
const selectorArgs = _allSelectorsArgs[selectorName]
const args = selectorArgs ? selectorArgs(state, selectorName) : []
extra.output = selector(state , ...args)