fix(isDeepEqual): Prevent redundant narrowing

[eranhirsch]

Fixes: #634

Narrowing the result of isDeepEqual is redundant when the type isn't effectively narrowed because it breaks the rejected/negated path which would be never.

This means that the common use case where both objects are of the same type and narrowing was meaningless, will now act as a regular boolean check.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 19d4457:

Sandbox	Source
remeda-example-vanilla	Configuration

[Brodzko]

Would probably be worth it to check that the type of data2 is not broken in either branch too? This turns out to be OK:

Suggested change

	if (isDeepEqual(data1, data2)) {
	expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
	} else {
	expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
	if (isDeepEqual(data1, data2)) {
	expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
	expectTypeOf(data2).toEqualTypeOf<{ a: number }>();
	} else {
	expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
	expectTypeOf(data2).toEqualTypeOf<{ a: number }>();

However if you do the data-last version, which should turn out exactly the same, it doesn't:

if (isDeepEqual(data2)(data1)) {
      expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
      expectTypeOf(data2).toEqualTypeOf<{ a: number }>();
    } else {
      expectTypeOf(data1).toEqualTypeOf<{ a: number }>();
      //           ^? `never`
      expectTypeOf(data2).toEqualTypeOf<{ a: number }>();
    }

[eranhirsch]

I don't understand what you meant regarding testing data2 as the type of data2 won't change after the call as it's not being narrowed so it will always be equal to what it was before the call.

As for the bigger issue with dataLast - it's actually pretty hard to test the negated/rejected result of the dataLast implementation because there's no implicit way to ask typescript to negate a type-predicate, outside of putting it in an if statement (where you'd use the dataFirst syntax for the reasons I explain further down). Even our attempts in functions like partition and isNot are hacks likely to break on some edge cases. This is because the else branch of a type-predicate is not Exclude<T, S>, as far as typescript narrows it. It's true that if your type is string | number and your type predicate would do is string then typescript would narrow the accepted branch to string and the rejected to number, but outside of unions the result is not well defined (this is the reason typescript doesn't export a utility type to compute this).

As for why your example fails, it's not because the type is broken; it's because that's not how to test it. When we built dataLast implementations in Remeda for cases like these, we have 2 options; one would define the generic on the curried function, as in:

function foo<T, ...>(...params): (data: T) => ...

and one where we push the definition of T to the returned function, as in:

function foo<...>(...params): <T>(data: T) => ...

They differ in when typescript needs to make a decision about what T is. The former requires typescript infer it when the curried function is called, and the latter only when the resulting function is called. The advantage of the former is that it allows the rest of the params to be influenced by the type inferred for T, so they depend on it. The advantage of the latter is that the resulting function becomes a kind of utility function that can take any T.

When we put functions in a pipe (or as an operand for a mapping function like filter), typescript has the information about how to type T from the pipe, so it works well with the former syntax; we rely on this and the dependency it puts on the rest of the params to provide stronger typing to the other params. This is useful for functions like "omit", "prop", and "pick" where we can enforce that the named prop is actually a prop of the object, and thus the IDE will provide typeahead for them and the compiler would fail on typos. When you use the latter approach you lose all of this.

Because our aim with dataLast is to make it useful for pipes and operands we use the former approach most of the time. But this means that the the use-case of a functional-programming-style curried utility function breaks as typescript is missing typing information to create a meaningful function.

This really long explanation is to say that when you called: isDeepEqual(data2)(data1) it wasn't equivalent to isDeepEqual(data1, data2), it was equivalent to:

const isDeepEqualToData2 = isDeepEqual(data2);
if (isDeepEqualToData2(data1)) {
  // ...
}

If you hover over isDeepEqualToData2 you'd see that the function is not a generic function that takes T, it's (data: unknown) => data is typeof data2. Because the data param now is unknown the typing now fails. You've effectively erased the typing from the type-predicate.

Sorry for the really long answer. We get contributions from people from all levels of experience, and I prefer to provide more than less so that these answers are as complete as possible so they can be used as a learning experience and to prevent extra back-and-forth with follow-up questions. I also use this as a surface to document my thought process in case others are reading this (maybe because they encounter a similar question and they git blame the file and find the PR in the future.

[Brodzko]

No worries at all - to the contrary, thank you for taking the time and going so in-depth, it is much appreciated! I think I understand now what the issue is; I didn't realise the difference you mentioned. So everything will work in pipes as expected, an in conditional predicates too 👍

Disregard that addition to tests, I think that must have been a brain fart on my side. I think I got the order of parameters confused in the original issue's reproduction and I though the type that is being used as a reference for comparison got screwed up there 🤷

[TkDodo]

🎉 This PR is included in version 1.58.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀