-
Notifications
You must be signed in to change notification settings - Fork 671
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
Allow passing in memoize functions directly to createSelector
.
#626
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9d7290f:
|
c991f9f
to
8ad544e
Compare
Thanks, I'll try to look at this over the weekend! |
fyi this is still on my list to review, but was at a conf last week and am also trying to sort out a couple other outstanding RTK PRs before I switch over to look at this. |
Not a problem at all, I know you're busy. I'm just trying to make everything easier for you to review so hopefully you can merge without any issues. Is there anything specific you would want me to do before you get a chance to review it? I'd be happy to help. |
Nothing I can think of immediately, other than "fix the TS types so they compile" :) |
Oh shoot. My apologies, I will fix that right away. |
… to override the initial memoize function passed in to createSelectorCreator
…reator` in order to simplify the types. I ran the all the type tests without it and it seems it can be removed without sacrificing any sort of type safety on any level. When calling `createSelectorCreator`, it doesn't need to know the type of the function it's memoizer at some point will memoize, it can be inferred later.
…ds in order to keep the types simple. I also removed `MemoizeOptions` from the `CreateSelectorFunction` interface and `createSelectorCreator` since its type can be derived directly from `MemoizeFunction`.
…ator I tried to get the types for `createSelectorCreator` to work with the new memoizeOptions type parameter without using overloads, but TS was still allowing passing in a second argument when the first argument is the memoizeOptions object. So I had to resort to using overloads. Might try again later to see if it's possible to make it work with only generics and conditionals.
…ncies` Will try to add more inline documentation and JSDocs to types and functions.
…ions I tested different variations of CreateSelectorOptions and CreateSelectorFunction. From what I've learned so far, it seems like CreateSelectorFunction can do fine with just one generic which is MemoizeFunction. It can derive the rest of the types as needed. CreateSelectorOptions now uses the MemoizeFunction generic type in addition to OverrideMemoizeFunction and OverrideArgsMemoizeFunction in order to create the type for the memoizeOptions object. Will investigate to see if CreateSelectorFunction can work with a `State` and `MemoizeFunction` generic type parameter.
UnknownFunction kind of sounds like it's a function that has parameters of type unknown. So for the sake of semantic consistency I will create another type called UnknownFunction which will have parameters of type unknown.
I also created UnknownFunction which has parameters of type unknown.
Will add more JSDoc to all types and function signatures.
The `Keys` generic type parameter was causing type mismatches for selectors created using other third party memoize methods such as `microMemoize` and `memoizeOne`. The types did not have the output selector fields attached to the selectors. Once I removed Keys, the type mismatch was resolved.
The type parameter for the first overload of `createSelectorCreator` looks quite messy, but it works. I will try and simplify it later to make it more readable.
There were some issues with `CreateSelectorOptions` not inferring the type of `memoizeOptions` or `argsMemoizeOptions` all the way and it would sometimes give implicit any errors on the `equalityCheck` and `resultEqualityCheck` functions.
Sometimes `CreateSelectorFunction` would not infer the type of the extra fields that memoizers return like `clearCache` for `defaultMemoize` or `isMemoized`. I added another generic type parameter called `ArgsMemoizeFunction` which is exactly what it sounds like, it is the type of `argsMemoize`. It was necessary to add this type parameter since it can directly impact the return type of the selector, specifically the extra memoizer fields that selectors return. e.g: clearCache, cache, isMemoized, etc.
*`runStabilityCheck` is the extracted logic of running stability checks inside `createSelectorCreator` made into its own standalone function. It made sense to segregate this piece of logic from the body of `createSelectorCreator` as it has its own purpose somewhat independently from the rest of the function. *`collectInputSelectorResults` is another piece of logic extracted from the body of `createSelectorCreator`. It only focuses on creating an array from the return values of input selectors.
- Made the following changes to enhance readability: * Add JSDocs to some of the types. * Rename some variables. * Rename some types. * Rename some generic type parameters. * Rename some parameter names. * Tweak some existing JSDocs.
Substituted `deepClone` since Node 16 does not support `structuredClone`.
95ef8e8
to
9f133f9
Compare
I've updated Now trying to actually get a sense of what all's changed in this PR :) |
src/utils.ts
Outdated
firstRun: boolean | ||
) => { | ||
return ( | ||
process.env.NODE_ENV !== 'production' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ We need the process.env.NODE_ENV
check to be wrapped around any calls to shouldRunInputStabilityCheck()
and runStabilityCheck()
. With the current code in the PR, those functions and the error messages are left in the production build artifact, which increases bundle size. We want the bundler to strip them out, and to get that flow control analysis to work it has to be wrapped around those calls in the parent function (ie createSelectorCreator
).
A bit uglier, and I totally understand the intent of "let's wrap this in a named util!" :) But gotta move that check out to make the prod build strip this logic out. (The inputStabilityCheck
part can stay here.)
Initial thoughts:
In fact, I think I've really only got two bits of feedback: First, per review comment, the The other one's trickier, and has to do with user DX for TS/VS Code hover previews. As an example: if you look at const selector = createSelector(
(state: { foo: string }) => 1,
(state: { bar: string }) => 2,
(...args) => 0
) Currently, the TS hover preview looks like this: It's a bit annoying (especially with the double With this PR, the preview is: which is a lot more verbose and doesn't even look like a function at all. Ideally, I'd want to see something akin to (made up): const selector: (state: {foo: string, bar: string}) => number
& {clearCache: () => void, recomputations: () => void, .......} In other words, a more human-readable representation that "this is a function with these args, and also has these fields attached". I have some long-left-over TS util types in I'm fine with merging this PR basically as-is (and I'll try to push the |
Okay, this looks amazing! Thank you! I'm going to go ahead and merge this, then do a couple other bits of cleanup and put out Let's do any hover preview changes and docs work in follow-up PRs. |
Thanks for all the feedback. I'm glad you mentioned the issue with the hover preview, because for the past couple of days, I've been looking for a way to simplify that as well. I will go ahead and submit PRs for some more JSDocs I was adding and the hover preview issue. |
Yep! Also FYI, we're now in a bizarre cyclical dep issue because I renamed I'm going to publish a new RTK beta shortly with fixes for this. |
I figured out how to solve the hover preview issue, but while doing so, I also (coincidentally) resolved the infinite type instantiation issue. The problem is it looks like resolving the hover preview issue, causes the inifnite type instantiation problem (to some extent). So I'm going to figure out if there is a reasonable middleground to reach. Also fixed some type issues related to |
@aryaemami59 that's certainly a step in the right direction! It'd be nice if we could get |
I will try again some more tomorrow, but I did have one question, is there a reason why we have an overload for |
No idea, actually! |
Yeah me neither, it doesn't make any sense. I'll see if I can get rid of it since the function itself has only one single behavior. |
This PR:
createSelector
.createSelectorCreator
signature overload.memoize
,memoizeOptions
,argsMemoize
andargsMemoizeOptions
tocreateSelector
.Runtime changes
createSelectorCreator
function overload that allows passing in anoptions
object to allow for more customization. Thememoize
property is mandatory, the rest are optional.memoize
andargsMemoize
to output selector fields.memoize
,argsMemoize
andargsMemoizeOptions
tooptions
argument insidecreateSelector
. This solves the issue of having to callcreateSelectorCreator
anytime we want to use an alternate memoize function.Why add
memoize
andargsMemoize
to output selector fields?It's simple, it allows us to swap out memoize functions, compose selectors from other selectors easier and it helps with debugging:
So now we can find out which memoize function would be fastest:
We get the following message in the console:
Or we can also do this:
Types
New types added
Add
UnknownMemoizer
type aliasUnknownMemoizer
is exactly what it sounds like. It's a function that takes another function as its first argument and returns it. It can also take anoptions
rest parameter.Add
Combiner
type alias(...args: SelectorResultArray<Selectors>) => Result
is seen too often in the codebase, so it made sense to shorten it by aliasing its type.Add
OmitIndexSignature
utility typeany
s from the return type of some memoizers. e.g:microMemoize
Add
IfNever
utility typememoizeOptions
based on whethermemoize
is provided or not. Same applies toargsMemoize
andargsMemoizeOptions
.Add
ExtractMemoizerFields
helper typeclearCache
Add
MemoizeOptionsFromParameters
helper typememoizeOptions
from the parameters ofmemoize
. Same applies toargsMemoize
andargsMemoizeOptions
.Add
OverrideMemoizeOptions
helper typememoizeOptions
based on whethermemoize
itself was overridden inside theoptions
argument ofcreateSelector
. Same applies toargsMemoize
andargsMemoizeOptions
.Add
FallbackIfNever
utility typeoptions
argument ofcreateSelector
. If nomemoize
is passed in, it will fallback to whatevermemoize
was originally passed intocreateSelectorCreator
. Same applies toargsMemoize
but sinceargsMemoize
is optional insidecreateSelectorCreator
, it will ultimately fallback todefaultMemoize
.Types changed
Rewrite
CreateSelectorFunction
This might be the biggest type change so far, yet it still pretty much functions like before. It does not break anything which means it is backwards-compatible. Here is all that has changed:
createSelectorCreator
signature overload.MemoizeFunction
which is almost the same as the oldMemoizeFunction
. The only difference is it now extendsUnknownMemoizer
and the type of the function it memoizes can be inferred.ArgsMemoizeFunction
which has been added due to the newoptions
argument increateSelector
and the new function overload forcreateSelectorCreator
. It is the type ofargsMemoize
which has a default oftypeof defaultMemoize
.combiner
parameter are still pretty much the same as before. Within the signature overloads provided byCreateSelectorFunction
, the 2 parts that have changed are theoptions
parameter and the return type specifically the output selector fields that are attached to the memoized selector e.g.:clearCache
options
parameter can now takememoize
,argsMemoize
andargsMemoizeOptions
in addition to the previously providedinputStabilityCheck
andmemoizeOptions
.memoize
, the type ofmemoizeOptions
will dynamically change to the type of the second parameter ofmemoize
. Ifmemoize
is not provided, the type ofmemoizeOptions
falls back to the second parameter of whatevermemoize
was initially passed in tocreateSelectorCreator
whether it was passed in as an object with the new signature overload or traditionally as a plain function.Here is an example:
The same relationship dynamic applies to
argsMemoize
andargsMemoizeOptions
. The only difference is sinceargsMemoize
is optional when passing in an object tocreateSelectorCreator
, the type ofargsMemoize
defaults totypeof defaultMemoize
and the type ofargsMemoizeOptions
is derived from the second parameter ofdefaultMemoize
.options
argument ofcreateSelector
:createSelectorCreator
type changesNew
createSelectorCreator
overload signaturecreateSelectorCreator
can now accept anoptions
object as its argument.Rewrite old
createSelectorCreator
function signaturecreateSelectorCreator
function signature works just like before except now it only takes one generic parameter ofMemoizeFunction
which is the type of the memoize function passed in. The type of thememoizeOptionsFromArgs
rest parameter is derived fromMemoizeFunction
.Change
DropFirst
toDropFirstParameter
Tail
, it made sense to change it from a type that drops the first item in an array to a type that drops the first parameter of a function.Rewrite
OutputSelectorFields
It now has 2 additional properties
memoize
andargsMemoize
. Instead of taking aCombiner
and aKeys
generic type parameter, it now takes 4 generic parameters:Selectors
: the input selectors. This enables us to get a strongly typeddependencies
property as well.Result
: the return value ofresultFunc
.MemoizeFunction
: type ofmemoize
. It is used to infer the correct type formemoizedResultFunc
and the newmemoize
output selector field.ArgsMemoizeFunction
: the memoize function used for the newargsMemoize
output selector field.Rewrite
OutputSelector
OutputSelectorFields
which in turn simplifies the return type ofCreateSelectorFunction
and should make it easier to maintain.Rewrite
OutputParametricSelector
Combiner
andKeys
generic type parameters as they are no longer needed. This is one of those types that I don't think is insanely popular or useful, so technically we could just remove it without it becoming too much of a problem. But I still rewrote it to match the new type definitions just in case.Rename
UnknownFunction
toAnyFunction
UnknownFunction
which used to be of type(...args: any[]) => any
toAnyFunction
. This was mostly due to the implied correlation between the wordAny
and parameters of typeany[]
. To avoid potential further confusion, I also addedUnknownFunction
which is an alias for(...args: unknown[]) => unknown
. This can be helpful for situations where you want to avoid usingany
.Other minor type related tweaks
never
in theParams
generic type parameter ofSelector
since a union ofany[]
andnever
is justany[]
.Selector
by localizing the conditional toParams
.createSelectorCreator
function body to increase readability.Explicit type imports/exports
import type
/export type
syntax.Deduplicate types
EqualityFn
was defined insideautotracking.ts
which has now been removed and replaced by an import ofEqualityFn
fromtypes.ts
.AnyFunction
originally defined asUnknownFunction
was defined insidets47-mergeParameters.ts
which after build would emitUnknownFunction$1
. It has now been removed and replaced byAnyFunction
imported fromtypes.ts
.Import and reuse
AnyFunction
wherever possibleAnyFunction
anywhere in the code where there is(...args: any[]) => any
Miscellaneous changes
createSelectorCreator
.eslintrc
as it conflicts withprettier
tsconfig
for type testsFile structure changes / relocation
StabilityCheck
andCreateSelectorOptions
intotypes.ts
.createSelectorCreator
into its own file.createStructuredSelector
into its own file.index.ts
to be a barrel file exporting everything.Add
utils.ts
tosrc
folderIt contains the following:
assertIsFunction
: Contains a small part of code insidecreateSelectorCreator
extracted to make the function body smaller and easier to read, and to add a small layer of type safety and reusability at the same time. Since we were doing a runtime check for a type of something and throwing an error on upon failure, the pattern looked very much like an assertion function which can help us kill 2 birds with one stone.ensureIsArray
: Another small part of code insidecreateSelectorCreator
. But since it is a pattern that we repeat, it does not hurt to turn it into a function that we can reuse.getDependencies
: Same function from before, the only difference is now upon failure, in order to be more explicit, it throws aTypeError
instead of anError
.collectInputSelectorResults
: Another piece of logic extracted from the body ofcreateSelectorCreator
. It only focuses on creating an array from the return values of input selectors.runStabilityCheck
: The extracted logic of running stability checks insidecreateSelectorCreator
made into its own standalone function.Just to make sure everything is now working as expected, I tested both the types and runtime behavior in my personal projects using
yalc
. I also wrote units tests and type tests for everything new that was added or changed so merging should not be a problem. Hope this helps, I would love to contribute more so please let me know if anything else is needed.Currently working on:
CreateSelectorFunction
which will introduce a breaking change.options
argument.TypedCreateSelectorFunction
, the current implementation only works with array args.vitest
.