Exclude invalid states at compile time about react-fast-compare HOT 13 CLOSED

Just chiming in since I was mentioned -- you want to think very carefully about the intent of this function. Consider this example:

var a: string | number;
var b: number | object;

declare function areEqual<T>(a: T, b: T): boolean;

// Error
areEqual(a, b);
// OK
a === b;

TypeScript uses a special relationship called comparability when using something like the === operator to figure out if it's "possibly true" that two types are comparable in a way that makes sense. There isn't a way to re-use this relationship from "user code" (i.e. in a generic function) so it can make sense to use a looser signature (any) to prevent false negatives.

from react-fast-compare.

Hmmm... that's an interesting observation, and being fairly new to TS, I'm curious how it fits in this library's situation which is that equal() really accepts any two random arguments of any type and gives a true/false back. The tests have all manner of dissimilar object type comparisons...

I'm curious what the best TS-appropriate answer is given this situation (the function is meant to handle dissimilar types, but a TS program should be programmed to never allow dissimilar types being compared...).

/cc @parkerziegler

from react-fast-compare.

awesome discussion @karol-majewski and @ryan-roemer! karol, do you have a real-world case where this would be more helpful? that might help take us out of the theoretical and into the practical.

from react-fast-compare.

@chrisbolin Yes, I do, and it's very simple — human errors.

shouldComponentUpdate(nextProps: Props): boolean {
  return !isEqual(nextProps.order, this.props.orders); // Always true
}

I work in e-commerce and our domain model is full of entities called order, orders, product or products. It's not uncommon for the API response to contain two similar-looking fields next to each other. The process of migrating our codebase to TypeScript revealed multiple instances of petty human errors like this one, which would not have happened if the developer knew what they were actually comparing.

from react-fast-compare.

Out of curiosity, what typedef does lodash isEqual have?

from react-fast-compare.

Lodash uses (value: any, other: any) => boolean. (Warning: a huge file)

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b44bc3fe3e8fcf087f338961e3e866a8f2e7b0a1/types/lodash/v3/index.d.ts#L10912-L10922

For comparison, Ramda uses <T>(a: T, b: T) => boolean.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b44bc3fe3e8fcf087f338961e3e866a8f2e7b0a1/types/ramda/index.d.ts#L596-L601

from react-fast-compare.

@ryan-roemer This makes a lot of sense to me. With this solution, TS theoretically will be able to determine at compile time that structural equality will fail based on differing interfaces. If the engineer knows ahead of time that structural equality will fail, it makes the need for an equality check moot (or alerts them they may need to correct something). My one concern would be that the type inference would fail in certain edge cases – I could forsee a circumstance in which the compiler incorrectly infers the interface of one of the comparators, resulting in a false negative. But, I have no real support for that idea. Just in my experience, type inference in TS can sometimes be incorrect.

The benefit of adding this would be that it encourages TS users to explicitly define interfaces and annotate data structures they're comparing. And I think @karol-majewski's use case is valid. This will catch user errors (I misspelled a key!), but allows the library to catch non-user errors (these two values evaluate differently, objects are not equal). That's my two cents 😄

from react-fast-compare.

the more I think about this, the more I like it.

@ryan-roemer what do you think about this...

• I make a github branch with the new, stricter Typescript interface.
• We have @karol-majewski test this in a real-world application, and see if we hit any false negatives (as @parkerziegler) mentioned
• If everything looks good we PR it and make a release

from react-fast-compare.

Also, there's an interesting issue how how do we version a type definition change? There will be no code changes, so that makes it feel like a patch release. But for TS users, it could literally break their compile, which makes it a major release. Is a minor bump a pragmatic approach? (cc @parkerziegler and @ryan-roemer)

from react-fast-compare.

It's definitely a major version if we do it for exactly the "break existing apps reason".

Odd thought -- could we provide two different typedef's for the same function, one loose and one tight?

from react-fast-compare.

@chrisbolin I like that plan! @ryan-roemer I suppose you may be able to do IInterfaceWeak | IInterfaceStrong for your exported API. That way, both interfaces would pass depending on how the developer used them. Not sure how kosher that approach is though.

Re: releases – JS users aren't affected by the type changes, TS users are. So doing a major release makes sense in that some users will get broken builds. So I'd go with your opinion on this one @ryan-roemer. JS users can upgrade if they want, or stay at current version. May be worth flagging in Release Notes when the version upgrade is TS-only vs. all users.

from react-fast-compare.

I like the way @RyanCavanaugh described the problem of versioning of TypeScript, which may be applied to the libraries providing types as well:

I think in practice what people do with semver is still sentimental versioning but dressed up with a little bit of fake formality [...] A major version is like I broke you on purpose, a minor version is if If I broke you, it's your fault and a patched version is I don't think this breaks anyone.

See TalkScript with the TypeScript Team from TSConf 2018.

As to the false negatives, the only example that comes to my mind is isEqual({}, '') which would pass without a warning, since both literals don't have any own properties, therefore they are assignable.

Providing two versions — weakly and strongly typed — usually happens via overloads, but in this case the most liberal overload is going to be chosen every time, defeating the purpose or having stronger types.

declare function equal<T>(a: T, b: T): boolean;
declare function equal(a: any, b: any): boolean;

equal(1, ''); // No error, because the most liberal overload is chosen.

from react-fast-compare.

thanks, @RyanCavanaugh!

In light of that, my vote is stick with (a: any, b: any) => boolean for now. Especially considering that example with union types

from react-fast-compare.