Key selectors composition

[sgrishchenko]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

What is the current behaviour? (You can also link to an open issue here)

Now if you try use cached selectors in dependecies, you need copy-paste all logic from their key selectors.

What is the new behaviour?

In this PR I propose solution for automatic key selectors composition of selector dependecies (will be composed only key selectors which returns strings).

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

This PR does not introduce a breaking change but changes some API:

• keySelector become optional (default keySelector is () => '')
• added new keySeparator option (default value is : like in here)
• added new composeKeySelectors option for disable this behavior (default value is true )

Other information:

When I use cached selector as dependency in simple selector, cached selector invalidates simple selector cache. To fix this I need to make simple selector cached (it is OK) and copy-paste key selector from dependecy (it is not OK). For example:

const dependency1 = createCachedSelector(
  state => state,
  (state, props) => props.id,
  (state, id) => state[id]
)((state, props) => props.id);

const dependency2 = createCachedSelector(
  state => state,
  (state, props) => props.otherId,
  (state, id) => state[id]
)((state, props) => props.otherId);

const cachedSelector = createCachedSelector(
  dependency1,
  dependency2,
  (first, second) => ({
    first,
    second
  })
)((state, props) => `${props.id}:${props.otherId}`);

All this leads to a terrible DX:

• So easy when adding a dependency, forget to update key selector
• If key selector from dependency contains some logic I need duplicate it
• If I refactor dependency key selector, I need update all dependent selectors

If compose key selectors automatically you can rewrite previous example like this:

const cachedSelector = createCachedSelector(
  dependency1,
  dependency2,
  (first, second) => ({
    first,
    second
  })
)();

As result key selector for cachedSelector will be

(state, props) => `:${props.id}:${props.otherId}`

If you introduce additional props, you can write something like this:

const cachedSelector = createCachedSelector(
  dependency1,
  dependency2,
  (state, props) => props.someProp
  (first, second, someProp) => ({
    first,
    second,
    someProp
  })
)((state, props) => props.someProp);

And result key selector again will be composed to this:

(state, props) => `${props.someProp}:${props.id}:${props.otherId}`

Please check if the PR fulfills these requirements:

• Tests for the changes have been added
• Docs have been added / updated

[coveralls]

Coverage remained the same at 100.0% when pulling dd5e5d8 on sgrishchenko:key-selectors-composition into 263c998 on toomuchdesign:master.

[toomuchdesign]

Hi @sgrishchenko, I see the point of supporting selectors' composability.

I'd like to consider the possibility of providing this feature as an external utility function. Something to directly provide as keySelector or maybe a sort of selectors composer like recompose does.

[sgrishchenko]

If I try provide external utility function, I receive something like this:

const keySelector = combineKeySelectors(
  dependency1,
  dependency2,
  (state, props) => props.someProp
)

const cachedSelector = createCachedSelector(
  dependency1,
  dependency2,
  (state, props) => props.someProp,
  (first, second, someProp) => ({
    first,
    second,
    someProp
  })
)(keySelector);

As you can see, I need again copy-paste dependencies and again so easy when adding a dependency, forget to update key selector. What if I create separate function createKeyComposedSelector(...) and encapsulate all additional logic in it?

[toomuchdesign]

It seems that the arguments of the hypothetical combineKeySelectors function are the same selectors provided as inputSelectors, right?

Not that I like this syntax, but as a proof of concept, might it be expressed like this?

const inputSelectors = [
  dependency1,
  dependency2,
  (state, props) => props.someProp,
];

const cachedSelector = createCachedSelector(
  inputSelectors,
  (first, second, someProp) => ({
    first,
    second,
    someProp
  })
)(combineKeySelectors(...inputSelectors));

[sgrishchenko]

At first, I can't pass all dependencies in combineKeySelectors function. There is three kinds of dependencies:

• Re-reselect selectors (or other selectors, that has keySelector property)
• Simple state selectors (like state => state.foo)
• Property selectors, which probably should be used in key selector (like (state, props) => props.someProp)

For this reason, I cannot distinguish state selectors from property selectors and I need do something like this:

const keySelectors = [
  dependency1,
  dependency2,
  (state, props) => props.someProp,
];

const cachedSelector = createCachedSelector(
  [
    state => state.foo,
    state => state.bar,
    ...keySelectors,
  ],
  (foo, bar, first, second, someProp) => ({
    foo,
    bar,
    first,
    second,
    someProp
  })
)(combineKeySelectors(...keySelectors));

Secondly, imagine that you future user of re-reselect library. You have two helpers: createCachedSelector and combineKeySelectors. As a good developer, the first thing you do is create a helper that combines these two helpers, because you don't want to copy-paste it every time. So why can't we do this out-of-box for library users?

Additionally, I understand your concern about granularity of this approach. I suggest to expose two new helper:

• createKeyCombinedSelector (high level for common usage)
• combineKeySelectors (low level for custom helpers)

What do you think about it?

[toomuchdesign]

For this reason, I cannot distinguish state selectors from property selectors and I need do something like this:

I've personally never dealt with such a scenario, therefore take what I say with a grain of salt: what if combineKeySelectors called each selector's keySelector (when existing) or the selector itself (when no keySelector is exposed) and chain only string | number results? It might have performance implications, of course.

It might get cumbersome (wrappers wrapping wrappers... mhh..), but I'd definitely go for the helper approach. createKeyCombinedSelector exposes slightly different API's (eg. no keySelector) and I consider polymorphic API's a no go in this context.

I'd also suggest to find a way of introducing these non-core feature gradually: maybe a separate re-reselect-util repo or exposing some experimental non stable API's. What would you go for @sgrishchenko?

Same goes for source code and tests: I'd try to keep core logic and the rest well separated. Rollup will think about shipping all together.

If there are still blurry areas about what these utility are supposed to do, we might consider writing down the documentation first.

Thanks for caring @sgrishchenko!
Greetings!

[toomuchdesign]

I just saw https://github.com/sgrishchenko/reselect-utils! Amazing job, there.

I'd keep this issue open as a remainder to update the docs to extensively mention reselect-utils.

[toomuchdesign]

I'm investigating the feasibility of such an API for the use case @sgrishchenko mentioned:

import createCachedSelector, {combineKeySelectors} from 're-reselect';

const cachedSelector = createCachedSelector(
  dependency1,
  dependency2,
  (state, props) => props.someProp
  (first, second, someProp) => ({
    first,
    second,
    someProp
  })
)(combineKeySelectors(options));

If we somehow extended the data provided to the keySelector with the inputSelectors array, combineKeySelectors should have everything it needs to automatically create the combined keySelector.

[toomuchdesign]

Closing in favour of #78. Thanks @sgrishchenko for the inspiring contribution!

[toomuchdesign]

Hi @dkaledin @veksa @sv1809 @eaglus,
with v3.3.0 introducing keySelectorCreator option, dynamic keySelector composition is now a thing.

With #78 I also tried to provide a keySelectorCreator function to ship with re-reselect, but I realised I don't have enough hands-on experience about dynamic selector composition to come up with a valuable solution.

So I'm inviting you to provide some real-world examples and possible API's to reason about the possibility of shipping such a keySelectorCreator function in a way which is generic enough to live in this library.

Thanks to you all. Cheers!

[mbellman]

@toomuchdesign @sgrishchenko Interesting following the back-and-forth here. I actually stumbled upon this thread through having misinterpreted the library similarly to the users in this other thread, and coming to realize that cache key selection didn't follow the mechanism I originally believed.

I think some of the frustrations expressed here would be mitigated through a design or option which just provided dependency function results to the cache key selector automatically. This would eliminate redundancies between the 'actual' selector and the cache key selector. Although, maybe this is precisely what @sgrishchenko was alluding to with his createKeyCombinedSelector idea. (I'm only just seeing/processing this discussion for the first time, so forgive me if I'm repeating notions already entertained.)

It is unfortunate that reselect does not expose its internal mechanism for building dependency results as an abstraction, meaning this library would have to replicate that behavior. However, allowing a key selector option to facilitate a predefined, "default" keySelectorCreator which behaved like this:

function createDependencyResultsKeySelector(keySelector, dependencies) {
  return function() {
    const dependencyResults = dependencies.map(dependency => dependency.apply(null, arguments));

    return keySelector.apply(null, dependencyResults);
  };
};

would allow usage like this:

const selectABC = createCachedSelector(
  selectA,
  selectB,
  selectC,
  (a, b, c) => compute(a, b, c)
)(
  (a, b, c) => `${a}:${b}:${c}`,
  { useDependencyResults: true }
);

...

// Boom
selectABC(state);

There would of course be (potentially) redundant work between re-reselect and reselect if a cached result was not found, and reselect proceeded on its merry way determining the dependency function results again. Ideally, the dependency functions would have themselves been created with appropriate memoization or caching, so the results wouldn't actually be recomputed with proper usage, but the process of running through the dependencies and building the list again would be.

I do want to emphasize the utility of allowing this library to use dependency results, rather than just initial selector arguments, to derive cache keys. It can be unwieldy to have to independently determine and pass in those values in cases where the selectors aren't normally written to take them, just to ensure that I can take advantage of re-reselect's superior caching ability. It also limits the ability of cached selectors to be used with createStructuredSelector in this fashion:

const mapStateToProps = createStructuredSelector({
  stuff: selectStuff,
  otherStuff: selectOtherStuff
});

...unless I want to write my key selectors to re-select (hah!) the "dependency results" manually, from state, to derive the right cache keys. Which I can. It's just a little leaner when done automatically. : - )

I can't see getting around a certain amount of awkwardness either way. For example, this is a little awkward:

/**
 * Using the 'extra argument' form: selectPersonByName(state, 'John')
 */
const selectPersonByName = createCachedSelector(
  selectPeople,
  // This 'dependency' just exists to forward the
  // selector's second argument to the result function
  (state, name) => name,
  (people, name) => people[name]
)(
  (_, name) => name
);

and this is a little awkward:

/**
 * Using the 'single state argument' form: selectCurrentStudentTestAverage(state)
 */
const selectCurrentStudentTestAverage = createCachedSelector(
  selectCurrentStudent,
  // This 'dependency' just exists to forward something
  // to the key selector (also, let's pretend currentStudent
  // does not already have the studentId property)
  selectCurrentStudentId,
  currentStudent => {
    const { testResults } = currentStudent;
    return testResults.reduce((acc, result) => acc + result, 0) / testResults.length;
  }
)(
  (_, studentId) => studentId,
  { useDependencyResults: true }
);

Anyway, that's my 2 cents!

[toomuchdesign]

Hi @mbellman,
I read your comment just now. Sorry, it was a quite busy time and I overlooked your message. Thanks for writing, though!

I 100% agree that the confusion about what argument are actually received by the keySelector is an issue hitting al lot of users. Including me: I realised that I made the same mistake in the last docs update. I just opened a PR.

As you mentioned, there are a few reasons why keySelector are provided with the initial selector arguments:

• Duplicated logic between reselect and re-reselect
• inputSelectors would be evaluated twice by reselect and re-reselect
• inputSelectors results are not known at declaration time (they're evaluated on runtime): using them as keySelectors arguments would have implications that would need further clarifications

I will open an issue to reason about the opportunity of adding the possibility of providing keySelectors with inputSelectors results instead of their same arguments.

I'm a bit reluctant to proceed lighthearted for 2 reasons:

• providing both option could make the library still harder to grasp
• TS typings complexity would increase exponentially

But I'm totally curious to better understand whether such a feature could provide real value.
Cheers! ✌️