Add ability to compare functions about react-fast-compare HOT 9 OPEN

Hi! Thanks for opening the issue. Comparing JS functions by string is a bit risky for reasons of closure/scopes. E.g., a contrived, but simplified example:

let x;
{
  const val = "hi";
  x = () => val;
}

let y;
{
  const val = "ho";
  y = () => val;
}

console.log("===", x === y); // => false
console.log("=== called", x() === y()); // => false
console.log("toString", x.toString() === y.toString()); // => true

I'm also looking at the other "classic deep equals" libraries like lodash isEqual() and dequal which don't do this or explicitly chose not to.

Thoughts?

Oh, and if we do decide this is the right course forward, the proper way to do this would be to try and open upstream in fast-deep-equal first and then bring in here (but they're a bit behind).

from react-fast-compare.

My PoV on this is if the same inline function is composed twice they should come out as equivalent. If there’s any change to the content of the function at all it shouldn’t be considered equal

from react-fast-compare.

Yeah, so the problem is we can't easily introspect if values and scope change the actual content or determine if functions are "pure" (which would allow this type of comparison).

In our case here () => val is not equivalent (due to scoped vals) because like if val was a something more substantive in the react context (an element) for a child prop, then we'd be rendering something differently entirely.

Put another way, all of the existing comparisons unequivocally never produce a spurious true -- the guarantee of the library is that while spurious falses may be produced (and thus cause like an unneeded re-render) we'll never have an incorrect true. And I can't think of a way to limit fn.toString() such that it will always provide accurate true values.. ?

from react-fast-compare.

Actually I’d argue they are the same because they’d reference the same scope variable

from react-fast-compare.

Yeah, but they're different scopes (which can happen in like different objects, etc. that are broadly different) and most importantly produce different rendered results which is that a fast equality check is meant to protect you against (a true allows you to short-circuit re-rendering).

Put another way, in my contrived example x is most definitely not equal to y for any programmatic usage :)

from react-fast-compare.

We need some sort of handling for functions though, otherwise this library is kinda useless for JSX with inline arrow functions (likely to be a common thing.) I know it’s not perfect but it should cover most common cases ☹️

from react-fast-compare.

Hmmm... I do wonder if maybe there's some option of like "allow unsafe function string comparison" or even a custom comparator fn we could take to allow end users to control their own destiny (enable the functionality and then police their own code for safe usage...)

from react-fast-compare.

I would advise against this as it's unsafe by default as @ryan-roemer suggested. It wouldn't be ideal for the library to be broken-by-default, but might be okay to allow people to opt into unsafe behavior. Here's a more realistic React example demonstrating this:

import { useCallback, useState } from "react";

export default function Counter() {
  const [counter, setCounter] = useState(0);

  return (
    <>
      <Button onClick={() => setCounter((value) => value + 1)}>+</button>
      <Button onClick={() => setCounter((value) => value - 1)}>-</button>
      <Button onClick={() => alert(`Value: ${counter}`)}>Get value</button>
    </>
  );
}

(Pretend <Button> is a component we've memoized using react-fast-compare and thus its onClick prop would be subject to this function comparison we're talking about.)

In the case of the + and - buttons, the suggested optimization is totally safe and would work fine: there's no way for those functions to behave differently on each render. But the same optimization would break the Get value button: the literal source code of the function doesn't change on each render, but the alerted value does (it encloses the counter variable). So, clicking it would always result in the alert Value: 0 even if the value were incremented or decremented.

In theory you could parse the function to see whether there are any non-local variable references within it (references like window, document, anything global, etc. would also need to be forbidden), but then you have the task of parsing a JavaScript function, and the "fast" part of "react-fast-compare" goes out the window (not to mention the bundle size implications). (Actually, this would even have to exclude the increment and decrement functions above, since those have a non-local setCounter reference, and react-fast-compare wouldn't know that that function doesn't change between renders.)

In addition to the non-local reference issue, this would likely also break anyone using higher-order functions, as even if you changed a function-value-parameter to another one that has a different toString() representation, the function returned by the HOF would not change its toString(), so you'd again have a bug.

e.g.

const handleClick = createClickHandler(flag ? doThis : doThat);

If you imagine what the createClickHandler might look like in this example, it doesn't include the actual source of doThis or doThat within it, it only treats them as a parameter, so its toString will always look something like the nested function in here:

function createClickHandler(callback) {
  return (...args) => {
    logClickEvent();
    callback(...args);
  }
}

Thus even if doThis and doThat have different toString() values, the resulting handleClick would not, even if flag changes between renders. Thus, if memoized by react-fast-compare with this unsafe function equality, you'd always be stuck with doThis or doThat and it'd never change.

If we do add an option to opt into this, I'd recommend making the danger very clear in documentation and naming, prefixing it with dangerous or unsafe, e.g. unsafeFunctionalStringEquality: true.

from react-fast-compare.

useEvent is attempting to solve this problem another way, but it's not here, yet.

You could introduce an option – I would vote for unsafeFunctionStringCompare 😄
I would probably avoid it, though. As demonstrated by the inability to identify the originating context, you can quickly run into issues. You wouldn't be able to tell if an anonymous event handler is passed in vs written inline, and it could be a nightmare to debug if you ended up in that situation.

Also keep in mind you would need to use Function.prototype.toString.call(a), not the provided .toString() to avoid malicious code.

const a = () => 'a';

// Misleading
const b = () => 'b';
b.toString = () => '() => \'a\'';

// Malicious
const c => runMaliciousCode();
c.toString = () => '() => \'a\'';

from react-fast-compare.