Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
markerikson merged 58 commits intoreduxjs:masterfromaryaemami59:memoizeOptions
Oct 28, 2023

Conversation

@aryaemami59
Copy link
Member

@aryaemami59aryaemami59 commentedSep 30, 2023
edited
Loading

This PR:

Runtime changes

  • AddcreateSelectorCreator function overload that allows passing in anoptions object to allow for more customization. Thememoize property is mandatory, the rest are optional.
  • Addmemoize andargsMemoize to output selector fields.
  • Allow passingmemoize,argsMemoize andargsMemoizeOptions tooptions argument insidecreateSelector. This solves the issue of having to callcreateSelectorCreator anytime we want to use an alternate memoize function.
  • So now instead of doing this:
constcreateSelectorWeakMap=createSelectorCreator(weakMapMemoize)constselectTodoIds=createSelectorWeakMap([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id))
  • We can do this:
constselectTodoIds=createSelector([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{memoize:weakMapMemoize})
Why addmemoize 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:

exportconstfindFastestSelector=<SextendsOutputSelector>(selector:S,  ...selectorArgs:Parameters<S>)=>{constmemoizeFuncs=[defaultMemoize,weakMapMemoize,autotrackMemoize]constresults=memoizeFuncs.map(memoizeFunc=>{constalternateSelector=createSelector(selector.dependenciesas[...SelectorArray],selector.resultFunc,{memoize:memoizeFunc})conststart=performance.now()//@ts-ignorealternateSelector(...selectorArgs)consttime=performance.now()-startreturn{name:memoizeFunc.name, time,selector:alternateSelector}})constfastest=results.reduce((minResult,currentResult)=>currentResult.time<minResult.time ?currentResult :minResult)constratios=results.filter(({ time})=>time!==fastest.time).map(({ time, name})=>`\x1b[33m \x1b[1m${time/fastest.time}\x1b[0m times faster than \x1b[34m${name}`)if(fastest.selector.memoize.name!==selector.memoize.name){console.warn(`The memoization method for \x1B[1;41m${selector.name}\x1B[0m is \x1B[31m${selector.memoize.name}\x1B[0m!\nChange it to \x1b[32m\x1b[1m${fastest.selector.memoize.name}\x1b[0m to be more efficient.\nYou should use \x1b[32m\x1b[1m${fastest.name}\x1b[0m because it is${ratios.join('\nand\n')}`)}return{ results, fastest}asconst}

So now we can find out which memoize function would be fastest:

constselectorAutotrack=createSelector([(state:State,id:number)=>id,(state:State)=>state.todos],(id,todos)=>todos.filter(todo=>todo.id===id),{memoize:autotrackMemoize})Object.defineProperty(selectorAutotrack,'name',{value:'selectorAutotrack'})//@ts-ignorefindFastestSelector(selectorAutotrack,state,1)

We get the following message in the console:

The memoization method for selectorAutotrack is autotrackMemoize!Change it to defaultMemoize to be more efficient.You should use defaultMemoize because it is 1.4709747668511033 timesfaster than weakMapMemoize and 9.652792674178125 times faster than autotrackMemoize

Or we can also do this:

exportfunctioncreateSelectorCreatorCurried(  ...args:Parameters<typeofcreateSelectorCreator>){constcreateSelector=createSelectorCreator(...args)return(...args:Parameters<ReturnType<typeofcreateSelectorCreator>>)=>{constselector=createSelector(...args)constcurriedSelector=selector.argsMemoize((...params)=>selector.argsMemoize((state:unknown)=>selector(state, ...params)))Object.assign(curriedSelector,selector)returncurriedSelector}}constcreateCurriedSelector=createSelectorCreatorCurried(defaultMemoize)constcurriedSelector=createCurriedSelector([(state:State,id:number)=>id,(state:State)=>state.todos],(id,todos)=>todos.filter(todo=>todo.id===id))curriedSelector(1)(state)// Returns [ { id: 1, completed: false } ]

Types

New types added

AddUnknownMemoizer type alias
  • UnknownMemoizer 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.
AddCombiner type alias
  • This was mostly because the pattern of(...args: SelectorResultArray<Selectors>) => Result is seen too often in the codebase, so it made sense to shorten it by aliasing its type.
AddOmitIndexSignature utility type
  • This is mainly used to remove explicitanys from the return type of some memoizers. e.g:microMemoize
AddIfNever utility type
  • It helps conditionally resolve the type ofmemoizeOptions based on whethermemoize is provided or not. Same applies toargsMemoize andargsMemoizeOptions.
AddExtractMemoizerFields helper type
  • A simple type alias that helps extract the extra properties that are attached to the return value of a memoizer. e.g.:clearCache
AddMemoizeOptionsFromParameters helper type
  • A simple type alias that helps extract the type ofmemoizeOptions from the parameters ofmemoize. Same applies toargsMemoize andargsMemoizeOptions.
AddOverrideMemoizeOptions helper type
  • It helps derive the type ofmemoizeOptions based on whethermemoize itself was overridden inside theoptions argument ofcreateSelector. Same applies toargsMemoize andargsMemoizeOptions.
AddFallbackIfNever utility type
  • It is used to help us detect whether a memoize function has been overridden inside theoptions 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

RewriteCreateSelectorFunction

  • 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:

    • It used to take 4 generic type parameters, now it takes 2. One of which was added due to the newcreateSelectorCreator signature overload.
    • First generic parameter isMemoizeFunction 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.
    • Second generic parameter isArgsMemoizeFunction 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.
    • The input selectors parameter and thecombiner 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
    • Theoptions parameter can now takememoize,argsMemoize andargsMemoizeOptions in addition to the previously providedinputStabilityCheck andmemoizeOptions.
    • If we pass in a value formemoize, 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:

    constcreateSelectorWeakMap=createSelectorCreator(weakMapMemoize)// This would result in a TS error.constselector1=createSelectorWeakMap([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{memoizeOptions:{maxSize:2}}// This causes the TS error.)// This on the other hand is fine.constselector2=createSelectorWeakMap([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{memoize:defaultMemoize,memoizeOptions:{maxSize:2}}// When `memoize` is changed to `defaultMemoize`, `memoizeOptions` can now be the same type as the options args in `defaultMemoize`.)
  • The same relationship dynamic applies toargsMemoize 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.

// This is fineconstcreateSelector1=createSelectorCreator({memoize:defaultMemoize,argsMemoizeOptions:{maxSize:2}})// This would result in a TS error.constcreateSelector2=createSelectorCreator({memoize:defaultMemoize,argsMemoize:weakMapMemoize,argsMemoizeOptions:{maxSize:2}// This causes the TS error.})
  • Same rules apply inside theoptions argument ofcreateSelector:
// This would result in a TS error.constselector1=createSelector([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{memoize:weakMapMemoize,memoizeOptions:{maxSize:2}// This causes the TS error.})// This on the other hand is fine.constselector2=createSelector([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{memoize:defaultMemoize,memoizeOptions:{maxSize:2}}// When `memoize` is set to `defaultMemoize`, `memoizeOptions` can now be the same type as the options args in `defaultMemoize`.)// This would result in a TS error.constselector3=createSelector([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{argsMemoize:weakMapMemoize,argsMemoizeOptions:{maxSize:2}// This causes the TS error.})// This on the other hand is fine.constselector4=createSelector([(state:State)=>state.todos],todos=>todos.map(todo=>todo.id),{argsMemoize:defaultMemoize,argsMemoizeOptions:{maxSize:2}}// When `argsMemoize` is set to `defaultMemoize`, `argsMemoizeOptions` can now be the same type as the options args in `defaultMemoize`.)

createSelectorCreator type changes

NewcreateSelectorCreator overload signature

  • createSelectorCreator can now accept anoptions object as its argument.
  • Here is what the new signature overload looks like:
exportfunctioncreateSelectorCreator<MemoizeFunctionextendsUnknownMemoizer,ArgsMemoizeFunctionextendsUnknownMemoizer=typeofdefaultMemoize>(options:{/** Overrides the global input stability check for the selector. */inputStabilityCheck?:StabilityCheck/** A function that accepts another function and returns it. This function is used to memoize `resultFunc`. */memoize:MemoizeFunction/** The memoizer function used to memoize the arguments of the selector. */argsMemoize?:FallbackIfNever<ArgsMemoizeFunction,typeofdefaultMemoize>/** Options object passed to `memoize` as the second argument. */memoizeOptions?:MemoizeOptionsFromParameters<MemoizeFunction>/** Options object passed to `argsMemoize` as the second argument. */argsMemoizeOptions?:OverrideMemoizeOptions<typeofdefaultMemoize,ArgsMemoizeFunction>}):CreateSelectorFunction<MemoizeFunction,ArgsMemoizeFunction>

Rewrite oldcreateSelectorCreator function signature

  • The initialcreateSelectorCreator 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.
  • Here is what it looks like now:
exportfunctioncreateSelectorCreator<MemoizeFunctionextendsUnknownMemoizer>(memoize:MemoizeFunction,  ...memoizeOptionsFromArgs:DropFirstParameter<MemoizeFunction>):CreateSelectorFunction<MemoizeFunction,typeofdefaultMemoize>
ChangeDropFirst toDropFirstParameter
  • Since this type originally was a duplicate ofTail, 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.
RewriteOutputSelectorFields
  • It now has 2 additional propertiesmemoize 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.
RewriteOutputSelector
  • It now takes the exact same generic type parameters asOutputSelectorFields which in turn simplifies the return type ofCreateSelectorFunction and should make it easier to maintain.
RewriteOutputParametricSelector
  • Remove theCombiner 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.
RenameUnknownFunction toAnyFunction
  • RenameUnknownFunction 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

  • Remove union withnever in theParams generic type parameter ofSelector since a union ofany[] andnever is justany[].
  • SimplifySelector by localizing the conditional toParams.
  • Add more explicit type assertions insidecreateSelectorCreator function body to increase readability.
Explicit type imports/exports
  • Separate type imports/exports from plain js imports/exports by using theimport type/export type syntax.
Deduplicate types
  • A duplicate ofEqualityFn was defined insideautotracking.ts which has now been removed and replaced by an import ofEqualityFn fromtypes.ts.
  • A duplicate ofAnyFunction 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 reuseAnyFunction wherever possible
  • Import and reuseAnyFunction anywhere in the code where there is(...args: any[]) => any

Miscellaneous changes

  • Add JSDocs tocreateSelectorCreator
  • Remove "indent" rule from.eslintrc as it conflicts withprettier
  • Add "@internal/" path totsconfig for type tests

File structure changes / relocation

  • MoveStabilityCheck andCreateSelectorOptions intotypes.ts.
  • MovecreateSelectorCreator into its own file.
  • MovecreateStructuredSelector into its own file.
  • Changeindex.ts to be a barrel file exporting everything.

Addutils.ts tosrc folder

It 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 usingyalc. 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:

  • Removing variadic function signatureCreateSelectorFunction which will introduce a breaking change.
  • Code mod to convert variadic args to array args. The existing code mod does not detect theoptions argument.
  • CreatingTypedCreateSelectorFunction, the current implementation only works with array args.
  • Writing more unit tests.
  • Writing more type tests.
  • Writing some benchmarks usingvitest.

@codesandbox-ci
Copy link

codesandbox-cibot commentedSep 30, 2023
edited
Loading

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:

SandboxSource
VanillaConfiguration
Vanilla TypescriptConfiguration

@aryaemami59aryaemami59 marked this pull request as ready for reviewSeptember 30, 2023 05:03
@markerikson
Copy link
Contributor

Thanks, I'll try to look at this over the weekend!

aryaemami59 reacted with heart emoji

@markerikson
Copy link
Contributor

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.

aryaemami59 reacted with heart emoji

@aryaemami59
Copy link
MemberAuthor

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.

@markerikson
Copy link
Contributor

Nothing I can think of immediately, other than "fix the TS types so they compile" :)

aryaemami59 reacted with laugh emoji

@aryaemami59
Copy link
MemberAuthor

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.

@aryaemami59aryaemami59 marked this pull request as draftOctober 24, 2023 21:34
@aryaemami59aryaemami59 marked this pull request as ready for reviewOctober 24, 2023 21:55
… 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.
@markerikson
Copy link
Contributor

I've updatedmaster to run the RTK CI example app tests against the latest RTK betas onrtk/v2.0-integration, and dropped support for TS <4.7. Rebased this PR againstmaster to pick those up.

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' &&
Copy link
Contributor

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.)

aryaemami59 reacted with heart emoji
@markerikson
Copy link
Contributor

Initial thoughts:

  • I like refactoring the codebase to move all the definitions out ofindex.ts, and leaving that as just the package exports
  • The JS logic seems reasonable
  • Ireally appreciate all the added JSDoc comments on all the types!
  • I haven't fully grokked or followed through on all the TS types changes, but things seem to make sense overall
  • Ilove the additional type, unit, and perf tests! Even without reading them carefully, that gives me a lot of confidence that your changes are working as intended.

In fact, I think I've really only got two bits of feedback:

First, per review comment, theprocess.env.NODE_ENV check needs to be at the parent function level to make sure this code gets eliminated during the prod build step

The other one's trickier, and has to do with user DX for TS/VS Code hover previews.

As an example: if you look attypescript_test/test.ts around line 1268, we've got this call:

constselector=createSelector((state:{foo:string})=>1,(state:{bar:string})=>2,(...args)=>0)

Currently, the TS hover preview looks like this:

image

It's a bit annoying (especially with the double{clearCache()} in there), but it shows you it's a function that takes an object as an argument.

With this PR, the preview is:

image

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 intypes.ts that are supposed to convince TS to expand types:Expand<T>,ExpandRecursive<T>,ComputeDeep<T>, andMapped<T>. I spent a few minutes trying to play with them and insert them in various spots aroundOutputSelector<T>. I was able to get the "it's just a function" part working, but that lost all knowledge of the attached selector fields.

I'm fine with merging this PR basically as-is (and I'll try to push theprocess.env.NODE_ENV changes myself shortly), but could you also take a stab at trying to get a more readable preview for selectors?

aryaemami59 reacted with heart emoji

@markerikson
Copy link
Contributor

Okay, this looksamazing! Thank you!

I'm going to go ahead and merge this, then do a couple other bits of cleanup and put out5.0.0-beta.0.

Let's do any hover preview changes and docs work in follow-up PRs.

aryaemami59 reacted with heart emoji

@aryaemami59
Copy link
MemberAuthor

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.

@markerikson
Copy link
Contributor

Yep! Also FYI, we're now in a bizarre cyclical dep issue because I renamedautotrackMemoize tounstable_autotrackMemoize and published that as Reselect 5.0.0-beta.0, but RTK beta tries to re-exportautotrackMemoize, and so the CI examples are now failing since that export doesn't exist :)

I'm going to publish a new RTK beta shortly with fixes for this.

aryaemami59 reacted with thumbs up emoji

@aryaemami59
Copy link
MemberAuthor

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 tocreateStructuredSelector.

markerikson reacted with eyes emoji

@aryaemami59
Copy link
MemberAuthor

selector-hover

I tried really hard to get VSCode to show as much info as possible in the hover preview without sacrificing too much TS performance / type safety. Do you think this suffices? I'm gonna have another go at it in the next few days either way, I just wanted to get your feedback on it if possible.

@markerikson
Copy link
Contributor

@aryaemami59 that's certainly a step in the right direction!

It'd be nice if we could getOutputSelectorFields simpler in some way, but that at least has the most critical info visible.

aryaemami59 reacted with thumbs up emoji

@aryaemami59
Copy link
MemberAuthor

@aryaemami59 that's certainly a step in the right direction!

It'd be nice if we could getOutputSelectorFields simpler in some way, but that at least has the most critical info visible.

I will try again some more tomorrow, but I did have one question, is there a reason why we have an overload forcreateStructuredSelector?

@markerikson
Copy link
Contributor

No idea, actually!

aryaemami59 reacted with laugh emoji

@aryaemami59
Copy link
MemberAuthor

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.

@aryaemami59aryaemami59 deleted the memoizeOptions branchOctober 31, 2023 16:32
@aryaemami59aryaemami59 restored the memoizeOptions branchOctober 31, 2023 16:38
@aryaemami59aryaemami59 deleted the memoizeOptions branchDecember 17, 2023 13:23
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@markeriksonmarkeriksonmarkerikson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@aryaemami59@markerikson

[8]ページ先頭

©2009-2025 Movatter.jp