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

suggestion: validate inputs being of type object, early bail-out if necessary #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ziir
Copy link

@ziir ziir commented Nov 6, 2024

Hi!
Thank you for maintaining this project.
We've been using it in production for the past 6months as our go-to shallow-equality-check utility.

However, we've recently been bit by a bug caused by our failure to ensure the input parameters we're passing to shallowEquals are indeed objects or arrays.

To illustrate, we've had a specific case where the input values were different numbers, unfortunately marked as any:

shallowEqual<any>(1, 2);
// true

shallowEqualObjects(1 as any, 2 as any);
// true

shallowEqualArrays(1 as any, 2 as any);
// true

I know this library is meant to be used with input values which the consumer knows are objects, and the library does not aim to provide a shallow-equality-check utility accounting for all the possible types & matching edge-cases.

However, I do believe it would be a valuable addition to introduce a bare minimum typeof-based validation at the right place, to avoid consumers treating some non-object-typeof values as being "shallow-equal", which is what this PR does.

I believe this direction still matches the project's philosophy since there are a couple of existing, similar-to-some-degree checks in the current implementation, such as loosely checking for falsy values in shallowEqualArrays notably.

Thank you for your consideration.

@ziir ziir force-pushed the suggestion/validate-inputs-type-object branch from 01e4cec to 6d6b67f Compare November 7, 2024 10:23
@Ariel-Moroshko
Copy link

I think this library should focus solely on performing shallow equality checks for arrays and objects, and it should disallow other types as inputs. Based on this, I think it would be beneficial to add a validation like this:

function shallowEqual<T extends Comparable>(a: T, b: T): boolean {
  const aIsArr = Array.isArray(a);
  const bIsArr = Array.isArray(b);

  if (aIsArr !== bIsArr) {
    return false;
  }

  if (aIsArr && bIsArr) {
    return shallowEqualArrays(a, b);
  }

  // NEW
  if (Object.prototype.toString.call(a) !== "[object Object]" || 
      Object.prototype.toString.call(b) !== "[object Object]") {
      throw new Error("arguments must be both arrays or both objects")
  }
  return shallowEqualObjects(a, b);
}

With this change, calling shallowEqual(1 as any, 2 as any) would throw an error instead of returning true.
Do you think this addition would solve the problem you've encountered?

Additionally, I believe shallowEqualArrays should avoid additional runtime checks, as its purpose is to compare two arrays directly—similar to how shallowEqualObjects is meant for objects only. Users who call shallowEqualArrays or shallowEqualObjects would be responsible for passing the correct types.

If a user prefers type checking to ensure safety and is okay with the runtime penalty, they can use shallowEqual.

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.

2 participants