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 features from Reselect@4.0.0 #60

Closed
wants to merge 5 commits into from

Conversation

sgrishchenko
Copy link
Contributor

@sgrishchenko sgrishchenko commented Dec 18, 2018

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

feature

What is the new behaviour?

supported recomputations and dependencies

see https://github.com/reduxjs/reselect/releases/tag/v4.0.0

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

I think so, because the reselect for similar changes made it.

Other information:

Please check if the PR fulfills these requirements:

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

@coveralls
Copy link

coveralls commented Dec 18, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 05fd607 on sgrishchenko:master into 63ab3c4 on toomuchdesign:master.

@toomuchdesign
Copy link
Owner

toomuchdesign commented Dec 23, 2018

Hi @sgrishchenko, thanks for your massive PR!

I'll try to split it up into different units to better understand what it introduces and if it needs any refinement.

I will dive into the code in a second moment. I need a few more time, for that.

I understand that the PR introduces the following 3 elements:

  • selector.dependencies (plus typings)
  • selector.recomputations / resetRecomputations (plus typings)
  • heterogeneous selectors

Side Note

If you have time, It'd be very helpful having the the commits of this PR split by feature more o less like the list above. 😸

selector.dependencies

New reselect v4 feature: reduxjs/reselect#251

selector.recomputations / resetRecomputations

This one is an old reselect feature never implemented at re-reselect level (see reselect source).

It's a property exposed for testing purpose. Since it's currently possible to retrieve the cached reselect selectors (and it's native props) with getMatchingSelector, may you help me finding a significative testing case for having the same prop provided at re-reselect level? I've never used this feature and I'm a bit out of context. :)

TS typings

Add relevant typings for:

  • selector.dependencies
  • selector.recomputations
  • selector.resetRecomputations

TS typings: heterogeneous selectors

I found I bit hard to understand what's happened here.

Looking forward to hearing back from you! ✋
Grretings!

@sgrishchenko
Copy link
Contributor Author

sgrishchenko commented Dec 23, 2018

Now the changes are already divided into 2 commit:

  • supporting recomputations and dependencies
  • add relevant typings for this + heterogeneous selectors + any number of uniform selectors

I think that the dependency feature is too small to put it into a separate commit.
I saw 2 questions:

Significative Testing Case For Recomputations

For example, how to check that the selector is recalculated only twice with this variant of the call:

const mySelector = ...

mySelector(state, { prop: 'val1' })
mySelector(state, { prop: 'val2' })
mySelector(state, { prop: 'val1' })
mySelector(state, { prop: 'val2' })

If you will use regular reselect selector, you will see next:

console.log(mySelector.recomputations()) // => 4

But if you will use re-reselect selector, you will see:

console.log(mySelector.recomputations()) // => 2

As you can see, I rewrote all the tests for using recomputations instead of jest.mock(). The problem is that in a real situation for a real selector, you cannot use a jest.mock() in a simple way. Recomputations greatly simplify the writing of test scenarios, when you want to understand how many times a resultFn will actually be called.
It will also be useful for reselect-tools project. It use this meta information in interactive mode.

Heterogeneous Selectors

For example, you have a very large project (maybe even a mono-repo). And each module does not know the full state interface.

// in first module
type FirstStateSegment = {
  firstState: ...
}

const firstSelector = (state: FirstStateSegment) => state.firstState

// in second module
type SecondStateSegment = {
  secondState: ...
}

const secondSelector = (state: SecondStateSegment) => state.secondState

And you need to write an integration module that uses the first module and the second module and their selestors.

// in third module
const thirdSelector = createCachedSelector(
  firstSelector,
  secondSelector,
  (firstState, secondState) => ...
)

Previously, from the point of view of typing, all selectors should have the same type of state. Heterogeneous selectors remove this restriction and now it is possible to create such selectors as the thirdSelector from the example above.

@toomuchdesign
Copy link
Owner

Thanks for the additional info, @sgrishchenko!

New features

I'd definitely go for introducing dependencies, recomputations and resetRecomputations. I'll take a moment to think about the possible performance implications of it.

Heterogeneous Selectors typings

As for "Heterogeneous Selectors" typings, I get the reason of it, but I'm afraid that adding 4000 lines of type definition to a 100 loc library might not be the best decision in terms of maintainability.

I've not much experience with TS but I understand that the main reason of such verbosity are the expressive limitations of the typing system itself, right?

I'd personally suggest to move the new Heterogeneous Selectors typings to a separate PR and reason about it from there. Maybe @xsburg ( 🙌) has something to say about it.

Reselect v4 dependency

Do we actually need forcing our consumers to use reselect v4? The new features might be implemented no matter of the reselect version.

I'd like our users not to be forced to switch to a newer version of Reselect if not strictly needed. :)

Thanks again!
Cheers!

@sgrishchenko
Copy link
Contributor Author

About performance implications: I had thought to disable recomputations in the production mode, but I looked at the implementation in the reselect and saw that they did not do that, and I decided not to make premature optimization.

About Heterogeneous Selectors typings: I am also not happy with everything in the current approach to typing and I have ideas how to reduce typing by about 4 times. But I decided not to do it yet to keep the analogy with the reselect. In the future, I plan to improve the typing in the reselect, and if my decision takes root, transfer this decision to this library. The idea is that non-parameterized selectors are a special case of parameterized selectors, and homogeneous selectors are special cases of heterogeneous selectors. But while in the current implementation to reduce the amount of code fails. If you look at the typing of a reselect, there are similar problems (although there are fewer lines because they do not use a prettier).

About separate PR: In short, I can not do it. The reason is that heterogeneous selectors now contain elements of the dependencies feature, but at this stage I have no guarantees that the dependencies feature will go to the library. It turns out that I need to make two pull requests: the first with features of heterogeneous selectors, the second with the addition of dependencies to heterogeneous selectors. I think all this will greatly delay the process, while users are already using heterogeneous selectors in the reselect, and I think it will be uncomfortable for them to find out that the re-reselect has not this feature.

About forcing to use reselect v4: I do not force). I only changed the dev dependency. Peer dependency remains the same and requires a reselect above the first version.

@toomuchdesign
Copy link
Owner

I see your points, now, thank you.
In order to have a better control over this PR, I'd still like to try an incremental approach.

I'd split your PR into 2 different PR's in order to be able to publish:

  1. a first minor release including dependencies, recomputations and resetRecomputations (with updated old typings) related tests and docs which I think won't take us long time to ship
  2. a second minor (or maybe major) release including the new typing system

Would you agree with this plan?
Cheers!

@sgrishchenko
Copy link
Contributor Author

Ok, thank you

…rs and any number of uniform selectors"

This reverts commit ef8d5a4
@sgrishchenko
Copy link
Contributor Author

sgrishchenko commented Dec 25, 2018

I split my PR into 2 different PR's, see #62 and #63

This was referenced Dec 25, 2018
… selectors and any number of uniform selectors""

This reverts commit 426a7c6
@toomuchdesign toomuchdesign mentioned this pull request Dec 26, 2018
2 tasks
@toomuchdesign
Copy link
Owner

Superseded by #62 and #63. Thank you @sgrishchenko!

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

Successfully merging this pull request may close these issues.

None yet

3 participants