-
Notifications
You must be signed in to change notification settings - Fork 79
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
Quality of life improvements #124
Conversation
@TrySound what do you think we should do with the existing flowtype? (I want to add a |
Thanks @TrySound! I'm trying to figure out how to give the callable property the same signature as ResultFn. I worked it out for typescript, but no success with flow yet. I suspect $Call will be needed but I haven't figured it out yet |
This is not right, but doing some exploration: // @flow
export type EqualityFn = (newArgs: mixed[], lastArgs: mixed[]) => boolean;
type ExtractReturnType = <TResult>(() => TResult) => TResult;
type ExtractArguments = <TArgs>((...args: TArgs) => mixed) => mixed;
// default export
declare export default function memoizeOne<ResultFn: (...any[]) => mixed>(
fn: ResultFn,
isEqual?: EqualityFn,
): {
(args: $Call<ExtractArguments, ResultFn>): $Call<ExtractReturnType, ResultFn>,
clear: () => void,
};
function add(first: number, second: number) {
return first + second;
}
const memoized = memoizeOne(add); I think extract return type is working, but extract arguments is way off Not sure if this is the best way to do things either 🤔 |
@TrySound I went for a sneaky flowtype which seems to work: // @flow
// These types are not as powerful as the TypeScript types, but they get the job done
export type EqualityFn = (newArgs: mixed[], lastArgs: mixed[]) => boolean;
// default export
declare export default function memoizeOne<ResultFn: (...any[]) => mixed>(
fn: ResultFn,
isEqual?: EqualityFn,
): ResultFn & { clear: () => void }; It is not as great as the |
You can try this out now with |
Release notesAt a glance🧹 New This release is a
New: Cache releasing with
|
The current plan is to release 6.x tomorrow 😲 |
.nvmrc
Outdated
14.17.6 |
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.
Change back to 16.x
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.
okay alex
Adding a collection of improvements to
memoize-one
. Still maintaining ie11 support at this stageTODO
.clear()
function property to memoized functions to allow for cache clearingEqualityFn
memoize-one
against other memoization librariesEqualityFn
EqualityFn
type.clear()
To be decided
MemoizedFn
type or just inline that type?Option 1: Use a
MemoizedFn
type(probably makes sense to export
MemoizedFn
)Option 2: Inline the new type
Went for option 1 as it reads nicer, as well as exposing a
MemoizedFn
type that people can more easily use if they want tominor
or amajor
major
. On balance I thought this was the least harmful. I know if I did it as aminor
somebody would flame me in an issue 😅type:module
in thepackage.json
?No.
Just going to leave it as is for now and can revisit
.length
? Respect function argument count #18No.
Code to add the
.length
property:This will
throw
on ie11. I could wrap the assignment in a try/catch. Right now this project still supports ie11, and this project is still leaned on super heavily in the JS community. I'm not looking to drop ie11 support just yet.I am currently thinking not to try to add
.length
(even in a try/catch). I don't want the memoized function to be different in different environments as this might cause some unexpected behaviours.name
and/or.displayName
property? (see post ie11 memoize-one #122)No.
Adding a
.name
/.displayName
property to a memoized function does not yield as good of results as I would have liked, and to support ie11 the.name
property assignment would need to be wrapped in atry/catch
; potentially further hurting performance.