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

ReScript-aware memo #111

Open
cometkim opened this issue Apr 21, 2024 · 3 comments
Open

ReScript-aware memo #111

cometkim opened this issue Apr 21, 2024 · 3 comments

Comments

@cometkim
Copy link
Member

cometkim commented Apr 21, 2024

The current definition of React.memo is:

@module("react")
external memo: component<'props> => component<'props> = "memo"

which is incorrect. React.memo takes an equality function as an optional second param

https://react.dev/reference/react/memo

memo(SomeComponent, arePropsEqual?)

Fortunately, this can be easily fixed without breaking


Customizing the second argument is rare in regular JS/TS projects, but not in ReScript.

ReScript's powerful type system represents boxed objects at runtime. Ironically, this makes memo almost useless in ReScript projects, since the default for memo compares shallow equality.

E.g. Playground

Users can easily make deep-equality function

let equal = \"="
// this is confusing btw...

generates

import * as Caml_obj from "./stdlib/caml_obj.js";

var equal = Caml_obj.equal;

However the deep-equal implementation is usually not what React users want. This will be a huge overhead when dealing with data that is not a persistent structure.

Assuming the user still wants the shallow-equal, we can try a much more optimized solution. The idea is simple, making recursive shallow-equal, using well-known structures.

builtin shallowEqual function in React
// https://github.com/facebook/react/blob/857ee8c/packages/shared/shallowEqual.js#L18

// `is` and `hasOwnProperty` are polyfill of `Object` static methods
function shallowEqual(objA: mixed, objB: mixed): boolean {
  if (is(objA, objB)) {
    return true;
  }

  if (
    typeof objA !== 'object' ||
    objA === null ||
    typeof objB !== 'object' ||
    objB === null
  ) {
    return false;
  }

  const keysA = Object.keys(objA);
  const keysB = Object.keys(objB);

  if (keysA.length !== keysB.length) {
    return false;
  }

  // Test for A's keys different from B.
  for (let i = 0; i < keysA.length; i++) {
    const currentKey = keysA[i];
    if (
      !hasOwnProperty.call(objB, currentKey) ||
      // $FlowFixMe[incompatible-use] lost refinement of `objB`
      !is(objA[currentKey], objB[currentKey])
    ) {
      return false;
    }
  }

  return true;
}
modified for ReScript outputs
function shallowEqualPolyvar(objA: polyvar, objB: polyvar) {
  if (objA.NAME !== objB.NAME) {
    return false;
  }
  return shallowEqual(objA, objB);
}

function shallowEqualVariant(objA: variant, objB: variant) {
  // Ok... this should be done by the compiler
  if (objA.TAG !== objB.TAG) {
    return false;
  }
  return shallowEqual(objA._0, objB._0)
}

function shallowEqual(objA: mixed, objB: mixed): boolean {
  if (is(objA, objB)) {
    return true;
  }

  if (
    typeof objA !== 'object' ||
    objA === null ||
    typeof objB !== 'object' ||
    objB === null
  ) {
    return false;
  }
  
  // We cannot skip check because there is no guarantee objs are record.
  // Or maybe we can make separate function for it.

  // isPolyvar should be provided by the compiler
  const isObjAPolyvar = isPolyvar(objA)
  const isObjBPolyvar = isPolyvar(objB)
  if (isObjAPolyvar !== isObjBPolyvar) {
    return false;
  } else if (isObjAPolyvar) {
    return shallowEqualPolyvar(objA, objB);
  }

  // isVariant should be provided by the compiler
  const isObjAVariant = isVariant(objA)
  const isObjBVariant = isVariant(objB)
  if (isObjAVariant !== isObjBVariant) {
    return false;
  } else if (isObjAVariant) {
    return shallowEqualVariant(objA, objB)
  }

  const keysA = Object.keys(objA);
  const keysB = Object.keys(objB);

  if (keysA.length !== keysB.length) {
    return false;
  }

  // Test for A's keys different from B.
  for (let i = 0; i < keysA.length; i++) {
    const currentKey = keysA[i];
    if (
      !hasOwnProperty.call(objB, currentKey) ||
      !is(objA[currentKey], objB[currentKey])
    ) {
      return false;
    }
  }

  return true;
}

it seems like compiler support is needed first 😅

@cometkim cometkim changed the title ReScript convential memo ReScript conventional memo Apr 21, 2024
@cknitt
Copy link
Member

cknitt commented Apr 21, 2024

I wouldn't say that the current binding is incorrect. React.memo is bound as two functions, one without and one with the optional equality function:

@module("react")
external memo: component<'props> => component<'props> = "memo"

@module("react")
external memoCustomCompareProps: (
  component<'props>,
  @uncurry ('props, 'props) => bool,
) => component<'props> = "memo"

This seems fine to me, although in ReScript 11 Uncurried Mode we could have a single function

@module("react")
external memo: (
  component<'props>,
  ~arePropsEqual: ('props, 'props) => bool=?,
) => component<'props> = "memo"

So the user can pass in an optional equality function that fits his use case, e.g., if one of the props is a variant (that is not option<'a> or @unboxed).

I don't really understand the "recursive shallow-equal" thing - isn't that a deep equal then, and doesn't it also come with quite a bit of performance overhead?

@cometkim
Copy link
Member Author

I don't really understand the "recursive shallow-equal" thing - isn't that a deep equal then, and doesn't it also come with quite a bit of performance overhead?

With a bit of reasoning, I think we can avoid a full comparison of values by only doing recursion on known wrapper structures like "TAG" (or custom), "_0". "NAME", "VAL", etc. Recursion is needed on only (non-external) nested variants (I guess they are likely to have persistent inner objects) and a typical record value would be 2-depth equality.

Or we could add something like @deriving(shallowEqual), but this is complex because it would require analyzing module dependencies.

@cometkim
Copy link
Member Author

Just opened rescript-lang/rescript#6738 as a minimal building block of this

@cometkim cometkim changed the title ReScript conventional memo ReScript-aware memo Apr 23, 2024
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

No branches or pull requests

2 participants