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

Quality of life improvements #124

Merged
merged 67 commits into from
Oct 20, 2021
Merged

Quality of life improvements #124

merged 67 commits into from
Oct 20, 2021

Conversation

alexreardon
Copy link
Owner

@alexreardon alexreardon commented Sep 27, 2021

Adding a collection of improvements to memoize-one. Still maintaining ie11 support at this stage

TODO

  • Add a .clear() function property to memoized functions to allow for cache clearing
  • Improving types of the memoized function
  • Improving types of EqualityFn
  • Adding a node script to compare the performance of memoize-one against other memoization libraries
  • Upgrade all dependencies
  • Shift over from TravisCI to Github actions
  • Add type tests to ensure types are working as expected
  • Add type tests for EqualityFn
  • update flow types
  • Release notes
  • README: new performance command
  • README: using the EqualityFn type
  • README: function properties
  • README: .clear()

To be decided

  • Use MemoizedFn type or just inline that type?

Option 1: Use a MemoizedFn type

(probably makes sense to export MemoizedFn)

export type MemoizedFn<TFunc extends (this: any, ...args: any[]) => any> = {
  clear: () => void;
  (this: ThisParameterType<TFunc>, ...args: Parameters<TFunc>): ReturnType<TFunc>;
};

function memoizeOne<TFunc extends (this: any, ...newArgs: any[]) => any>(
  resultFn: TFunc,
  isEqual: EqualityFn<TFunc> = areInputsEqual,
): MemoizedFn<TFunc> {
/*...*/
}

Option 2: Inline the new type

function memoizeOne<TFunc extends (this: any, ...newArgs: any[]) => any>(
  resultFn: TFunc,
  isEqual: EqualityFn<TFunc> = areInputsEqual,
): {
  clear: () => void;
  (this: ThisParameterType<TFunc>, ...args: Parameters<TFunc>): ReturnType<TFunc>;
} {
/*...*/
}

Went for option 1 as it reads nicer, as well as exposing a MemoizedFn type that people can more easily use if they want to

  • Should this version be a minor or a major

major. On balance I thought this was the least harmful. I know if I did it as a minor somebody would flame me in an issue 😅

  • Do we need type:module in the package.json?

No.

Just going to leave it as is for now and can revisit

No.

Code to add the .length property:

Object.defineProperty(memoized, 'length', {
  writable: false,
  enumerable: false,
  configurable: true,
  value: resultFn.length,
});

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

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 a try/catch; potentially further hurting performance.

@alexreardon
Copy link
Owner Author

@TrySound what do you think we should do with the existing flowtype? (I want to add a .clear() property to the ResultFn type)

@TrySound
Copy link
Contributor

@alexreardon
Copy link
Owner Author

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

https://flow.org/en/docs/types/utilities/#toc-call

@alexreardon
Copy link
Owner Author

alexreardon commented Oct 13, 2021

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 🤔

@alexreardon
Copy link
Owner Author

alexreardon commented Oct 15, 2021

@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 TypeScript type, but it at least adds the .clear() property

@alexreardon
Copy link
Owner Author

You can try this out now with memoize-one@6.0.0-beta.1

@alexreardon
Copy link
Owner Author

alexreardon commented Oct 18, 2021

Release notes

At a glance

🧹 New .clear() function to remove the current memoization cache
🏋️‍♀️ Stronger types
🥰 Improved documentation

This release is a major, but there are no behaviour or API changes. The major is to reflect that some of the TypeScript types have been tightened which might cause some peoples TypeScript builds to break.

PSA: memoize-one will soon™️ be dropping ie11 support
More details

New: Cache releasing with .clear() 🧹

A .clear() property is now added to memoized functions to allow you to clear it's memoization cache

This is helpful if you want to:

  • Release memory
  • Allow the underlying function to be called again without having to change arguments
import memoizeOne from 'memoize-one';

function add(a: number, b: number): number {
  return a + b;
}

const memoizedAdd = memoizeOne(add);

// first call - not memoized
const first = memoizedAdd(1, 2);

// second call - cache hit (underlying function not called)
const second = memoizedAdd(1, 2);

// 👋 clearing memoization cache
memoizedAdd.clear();

// third call - not memoized (cache was cleared)
const third = memoizedAdd(1, 2);

Improved: memoized function type

There are no API changes to memoize-one, this is merely a typing improvement

Our previous type for a memoized function was simple:

Previous

export declare type EqualityFn = (newArgs: any[], lastArgs: any[]) => boolean;

declare function memoizeOne<ResultFn extends (this: any, ...newArgs: any[]) => ReturnType<ResultFn>>(resultFn: ResultFn, isEqual?: EqualityFn): ResultFn;

A memoized function claimed to be the same type (ResultFn) as the original function. This was not true, as memoize-one does not copy of any existing object properties on the original function (TFunc)

Updated

export declare type MemoizedFn<TFunc extends (this: any, ...args: any[]) => any> = {
  clear: () => void;
  (this: ThisParameterType<TFunc>, ...args: Parameters<TFunc>): ReturnType<TFunc>;
};

declare function memoizeOne<TFunc extends (this: any, ...newArgs: any[]) => any>(
  resultFn: TFunc,
  isEqual?: EqualityFn<TFunc>,
): MemoizedFn<TFunc>;

A memoized function returns the same callable signature as the original function (TFunc → was ResultFn), but it makes it clear that no function object properties on the original function (TFunc) are being carried forward. The memoized function also now includes a .clear() function object property

If you want to continue to use the old types where the memoized function is the same type as the function being memoized, you can achieve this by casting the type of your memoized function:

function add(first: number, second: number): number {
  return first + second;
}
// a type that matches our add function
type AddFn = (first: number, second: number) => number;

// type of memoized will be MemoizedFn<typeof add>
const memoized = memoize(add);

// option 1
const memoized: typeof add = memoize(add);

// option 2
const memoized: AddFn = memoize(add);

// option 3
const memoized = memoize(add) as typeof add;

// option 4
const memoized = memoize(add) as AddFn;

This type change has been labelled as a patch (fix) as the previous type was not correct. However, you could consider it a major given that the new type is narrower than before

Improved: equality function type

There are no API changes to equality functions, this is merely a typing improvement

Previous

export type EqualityFn = (newArgs: any[], lastArgs: any[]) => boolean;

Current

export type EqualityFn<TFunc extends (...args: any[]) => any> = (
  newArgs: Parameters<TFunc>,
  lastArgs: Parameters<TFunc>,
) => boolean;

This looks a little scary, but it is pretty neat! It means that you can dramatically improve the type safety of your custom equality functions if you want to.

If you are not using a custom equality function

No changes for you!

If you are using a custom equality function

Most people will not be impacted!

This type tightening allows you to be a lot stricter with the shape of your functions passed in as equality functions. If you are using generic equality functions such as lodash.isequal their types are loose and there is nothing you will need to do. But if you want to write more efficient and typesafe equality functions, you are in for a treat.

An example of what things looked like in 5.x

import memoize, { EqualityFn } from "memoize-one";

type Person = {
  id: string;
  name: string;
};

function invite(person: Person) {
  // This could do something fancy, but just keeping things basic
  console.log("invited:", person.name);
}

// Yuck, we don't know anything about the args
// Note: don't really need the `EqualityFn` type as it is just:
// `type EqualityFn = (newArgs: any[], lastArgs: any[]) => boolean;`
const isEqual: EqualityFn = (newArgs: any[], lastArgs: any[]): boolean => {
  // Yuck #2: we have to cast here
  // We would also be creating a bug if isEqual is used on a function
  // that has no arguments
  const first = newArgs[0] as Person;
  const second = lastArgs[0] as Person;
  return first.id === second.id;
};

const memoized = memoize(invite, isEqual);

const alex: Person = {
  name: "Alex",
  id: "11111"
};

memoized(alex);
// Won't do anything as `alex` has the same id as the last `alex`
memoized(alex);

You can play with this example on codesandbox.io

The same example in 6.x

import memoize, { EqualityFn } from "memoize-one";

type Person = {
  id: string;
  name: string;
};

function invite(person: Person) {
  console.log("invited:", person.name);
}

// Yum: we know that newArgs + lastArgs are the tuple `[Person]`
const isEqual: EqualityFn<typeof invite> = ([first], [second]): boolean => {
  return first.id === second.id;
};

const memoized = memoize(invite, isEqual);

const alex: Person = {
  name: "Alex",
  id: "11111"
};

memoized(alex);
// Won't do anything as `alex` has the same id as the last `alex`
memoized(alex);

// When declared inline, our isEqual function has the correct types inferred
const inferred = memoize(invite, function isEqual([first], [second]): boolean {
  return first.id === second.id;
});

You can play with this example on codesandbox.io

There are a few cases where this could cause your TypeScript types to start failing, so this change has been listed as a major

Improved: Documentation

  • Cleanup and audit of examples
  • Added documentation for .clear()
  • Added documentation about function properties and the .length property
  • Updated performance benchmarks

Internal code health

  • Now on TypeScript@4.4.3
  • Moved from TravisCI to Github workflows
  • Upgraded devDependencies

@alexreardon
Copy link
Owner Author

The current plan is to release 6.x tomorrow 😲

.nvmrc Outdated
14.17.6
Copy link
Owner Author

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay alex

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.

3 participants