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

Support for re-reselect #21

Open
bdurrer opened this issue Jul 2, 2019 · 4 comments
Open

Support for re-reselect #21

bdurrer opened this issue Jul 2, 2019 · 4 comments

Comments

@bdurrer
Copy link

bdurrer commented Jul 2, 2019

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
Copy link
Owner

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
Copy link

parkerault commented Oct 7, 2019

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:

  • A defaultProps parameter to registerSelectors would require a key-value pair for every unique key used in the application. This would also result in the same key-value pair being used for every selector that consumes that key, which may not work.
  • Adding a defaultProps value or a default parameter value for each "props" selector (i.e. (__, props = { prop: 1 }) => props.prop) to every multi-arity selector would allow you to specify the default props individually, but would require even more work than the single defaultProps parameter to registerSelectors

@skortchmark9
Copy link
Owner

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
Copy link
Author

bdurrer commented Oct 8, 2019

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)

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

No branches or pull requests

3 participants