- Notifications
You must be signed in to change notification settings - Fork666
Allow passing in memoize functions directly tocreateSelector.#626
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codesandbox-cibot commentedSep 30, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This pull request is automatically built and testable inCodeSandbox. To see build info of the built libraries, clickhere or the icon next to each commit SHA. Latest deployment of this branch, based on commit9d7290f:
|
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`.
…atorI 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.
…ionsI 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.
Substituted `deepClone` since Node 16 does not support `structuredClone`.
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 theprocess.env.NODE_ENV check to be wrappedaround any calls toshouldRunInputStabilityCheck() andrunStabilityCheck(). 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 (iecreateSelectorCreator).
A bit uglier, and I totally understand the intent of "let's wrap this in a named util!" :) But gotta movethat check out to make the prod build strip this logic out. (TheinputStabilityCheck 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 constselector=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 evenlook like a function at all. Ideally, I'd want to see something akin to (made up): constselector:(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 looksamazing! 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. |



Uh oh!
There was an error while loading.Please reload this page.
This PR:
createSelector.createSelectorCreatorsignature overload.memoize,memoizeOptions,argsMemoizeandargsMemoizeOptionstocreateSelector.Runtime changes
createSelectorCreatorfunction overload that allows passing in anoptionsobject to allow for more customization. Thememoizeproperty is mandatory, the rest are optional.memoizeandargsMemoizeto output selector fields.memoize,argsMemoizeandargsMemoizeOptionstooptionsargument insidecreateSelector. This solves the issue of having to callcreateSelectorCreatoranytime we want to use an alternate memoize function.Why add
memoizeandargsMemoizeto 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
UnknownMemoizertype aliasUnknownMemoizeris exactly what it sounds like. It's a function that takes another function as its first argument and returns it. It can also take anoptionsrest parameter.Add
Combinertype alias(...args: SelectorResultArray<Selectors>) => Resultis seen too often in the codebase, so it made sense to shorten it by aliasing its type.Add
OmitIndexSignatureutility typeanys from the return type of some memoizers. e.g:microMemoizeAdd
IfNeverutility typememoizeOptionsbased on whethermemoizeis provided or not. Same applies toargsMemoizeandargsMemoizeOptions.Add
ExtractMemoizerFieldshelper typeclearCacheAdd
MemoizeOptionsFromParametershelper typememoizeOptionsfrom the parameters ofmemoize. Same applies toargsMemoizeandargsMemoizeOptions.Add
OverrideMemoizeOptionshelper typememoizeOptionsbased on whethermemoizeitself was overridden inside theoptionsargument ofcreateSelector. Same applies toargsMemoizeandargsMemoizeOptions.Add
FallbackIfNeverutility typeoptionsargument ofcreateSelector. If nomemoizeis passed in, it will fallback to whatevermemoizewas originally passed intocreateSelectorCreator. Same applies toargsMemoizebut sinceargsMemoizeis optional insidecreateSelectorCreator, it will ultimately fallback todefaultMemoize.Types changed
Rewrite
CreateSelectorFunctionThis 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:
createSelectorCreatorsignature overload.MemoizeFunctionwhich is almost the same as the oldMemoizeFunction. The only difference is it now extendsUnknownMemoizerand the type of the function it memoizes can be inferred.ArgsMemoizeFunctionwhich has been added due to the newoptionsargument increateSelectorand the new function overload forcreateSelectorCreator. It is the type ofargsMemoizewhich has a default oftypeof defaultMemoize.combinerparameter are still pretty much the same as before. Within the signature overloads provided byCreateSelectorFunction, the 2 parts that have changed are theoptionsparameter and the return type specifically the output selector fields that are attached to the memoized selector e.g.:clearCacheoptionsparameter can now takememoize,argsMemoizeandargsMemoizeOptionsin addition to the previously providedinputStabilityCheckandmemoizeOptions.memoize, the type ofmemoizeOptionswill dynamically change to the type of the second parameter ofmemoize. Ifmemoizeis not provided, the type ofmemoizeOptionsfalls back to the second parameter of whatevermemoizewas initially passed in tocreateSelectorCreatorwhether 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
argsMemoizeandargsMemoizeOptions. The only difference is sinceargsMemoizeis optional when passing in an object tocreateSelectorCreator, the type ofargsMemoizedefaults totypeof defaultMemoizeand the type ofargsMemoizeOptionsis derived from the second parameter ofdefaultMemoize.optionsargument ofcreateSelector:createSelectorCreatortype changesNew
createSelectorCreatoroverload signaturecreateSelectorCreatorcan now accept anoptionsobject as its argument.Rewrite old
createSelectorCreatorfunction signaturecreateSelectorCreatorfunction signature works just like before except now it only takes one generic parameter ofMemoizeFunctionwhich is the type of the memoize function passed in. The type of thememoizeOptionsFromArgsrest parameter is derived fromMemoizeFunction.Change
DropFirsttoDropFirstParameterTail, 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
OutputSelectorFieldsIt now has 2 additional properties
memoizeandargsMemoize. Instead of taking aCombinerand aKeysgeneric type parameter, it now takes 4 generic parameters:Selectors: the input selectors. This enables us to get a strongly typeddependenciesproperty as well.Result: the return value ofresultFunc.MemoizeFunction: type ofmemoize. It is used to infer the correct type formemoizedResultFuncand the newmemoizeoutput selector field.ArgsMemoizeFunction: the memoize function used for the newargsMemoizeoutput selector field.Rewrite
OutputSelectorOutputSelectorFieldswhich in turn simplifies the return type ofCreateSelectorFunctionand should make it easier to maintain.Rewrite
OutputParametricSelectorCombinerandKeysgeneric 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
UnknownFunctiontoAnyFunctionUnknownFunctionwhich used to be of type(...args: any[]) => anytoAnyFunction. This was mostly due to the implied correlation between the wordAnyand parameters of typeany[]. To avoid potential further confusion, I also addedUnknownFunctionwhich is an alias for(...args: unknown[]) => unknown. This can be helpful for situations where you want to avoid usingany.Other minor type related tweaks
neverin theParamsgeneric type parameter ofSelectorsince a union ofany[]andneveris justany[].Selectorby localizing the conditional toParams.createSelectorCreatorfunction body to increase readability.Explicit type imports/exports
import type/export typesyntax.Deduplicate types
EqualityFnwas defined insideautotracking.tswhich has now been removed and replaced by an import ofEqualityFnfromtypes.ts.AnyFunctionoriginally defined asUnknownFunctionwas defined insidets47-mergeParameters.tswhich after build would emitUnknownFunction$1. It has now been removed and replaced byAnyFunctionimported fromtypes.ts.Import and reuse
AnyFunctionwherever possibleAnyFunctionanywhere in the code where there is(...args: any[]) => anyMiscellaneous changes
createSelectorCreator.eslintrcas it conflicts withprettiertsconfigfor type testsFile structure changes / relocation
StabilityCheckandCreateSelectorOptionsintotypes.ts.createSelectorCreatorinto its own file.createStructuredSelectorinto its own file.index.tsto be a barrel file exporting everything.Add
utils.tstosrcfolderIt contains the following:
assertIsFunction: Contains a small part of code insidecreateSelectorCreatorextracted 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 aTypeErrorinstead 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 insidecreateSelectorCreatormade 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:
CreateSelectorFunctionwhich will introduce a breaking change.optionsargument.TypedCreateSelectorFunction, the current implementation only works with array args.vitest.