refactor: remove lodash

[re-taro]

What:

Replace lodash logic and remove it from deps

Why:

To resolve this issue -> #592

How:

https://youmightnotneed.com/lodash/

Checklist:

• Documentation N/A
• Tests
• Updated Type Definitions N/A
• Ready to be merged
  

[gnapse]

Looks good. Though, I'm suggesting an improvement to not lose readability of the code.

[gnapse]

The one thing lost here is readability. It is easier to read the previous version.

What I suggest is that you create a function isEqualWith (or isEqualWithArraysAsSets) in the utils.js module, that would encapsulate this functionality. Makes sense?

[gnapse]

And reuse the isEqualWith function here.

[gnapse]

I meant to request changes instead, due to my suggestion. Sorry.

[re-taro]

Good suggestion. I will revise it!

[gnapse]

Note

Before diving into my long comment below, a caveat: maybe our isEqual implementation does not need the features that I'm highlighting below that are missing compared to lodash's isEqual. Let me know.

I'm now having second thoughts about discarding the use of lodash's isEqual entirely. For a few reasons:

• It is a battle-tested implementation that covers edge cases that we might not be covering here (see below for one of these cases).
• It is not featured in the website https://youmightnotneed.com/lodash/, meaning that they do not have any recommended in-house implementation that replaces the lodash function.

Edge cases not covered here

I think that the function in lodash is also able to compare two objects for equality (i.e. looping over the keys and comparing that equal keys have equal values). Furthermore, that last part about comparing if equal keys have equal values is probably recursive: that is, it calls itself with the values of the two keys:

I asked ChatGPT and this is what it came up with (preserving our array-as-sets comparison as well):

function isEqual(obj1, obj2) {
  // Check if both objects are arrays
  if (Array.isArray(obj1) && Array.isArray(obj2)) {
    // Sort the arrays before comparison
    const arraySet1 = [...new Set(arr1)]
    const arraySet2 = [...new Set(arr2)]

    // Compare the sorted arrays
    return arraySet1.every(v => arraySet2.has(v))
  }

  // Check if both arguments are objects
  if (typeof obj1 === 'object' && typeof obj2 === 'object') {
    // Get the keys of the objects
    const keys1 = Object.keys(obj1);
    const keys2 = Object.keys(obj2);

    // Check if the number of keys is the same
    if (keys1.length !== keys2.length) {
      return false;
    }

    // Check if all keys and their corresponding values are equal
    for (let key of keys1) {
      // Recursive call to isEqual for nested objects
      if (!isEqual(obj1[key], obj2[key])) {
        return false;
      }
    }

    // If all keys and values are equal, return true
    return true;
  }
  
  // If the arguments are not objects, compare them directly
  return obj1 === obj2;
}

And even then, probably we'd need to come up with a way to compare the array elements via isEqual recursively, which this implementation is not doing.

[re-taro]

I have one suggestion. However, this proposal is not a complete solution to the original issue.

• Use lodash-es
• Re-implement the isEqual equivalent of lodash
  • It is not maintainable.

[gnapse]

I agree we can keep isEqual. If it's via lodash-es that's great. The goal should not blindly be about removing lodash entirely. Specially given that jest-dom is not normally a dependency that gets into a web app's bundle, given that it is mostly a dev dependency only.

I'd appreciate if you can do that change and we can merge this. We can leave trace in this PR and the original issue, that it was closed without fully removing lodash, but minimizing its usage to where is really needed.

Thanks!

[re-taro]

Sorry, this is just a simple question I have, but in the original code, where isEqualWith was used, what could be in the values being compared? Because the documentation for lodash.isEqual says This method supports comparing arrays, array buffers, booleans, date objects, error objects, maps, numbers, Object objects, regexes, sets, strings, symbols, and typed arrays. I personally feel that this is overkill, although I do not have a full understanding of the code or design.
I would appreciate it if you could let me know so that I can provide a better code.

[gnapse]

I know that the lodash version is surely covering use cases that we do not need, but on the flip side, a custom-made alternative may miss cases that we do need to cover. I'm not 100% sure that our test cases are iron-clad in covering all potential use cases. Hence, I'd rather keep isEqual, even if it means not totally ditching lodash.

I do not think that the goal should be to blindly avoid lodash. Specially in this library that's not going to be part of a web app bundle. So I'd rather stay on the safe side, and keep it.

Makes sense?

[re-taro]

I see, I'm certainly in favor of taking them down safely for the sake of trust.

[gnapse]

Thanks!

[github-actions]

🎉 This PR is included in version 6.4.3 🎉

The release is available on:

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

Your semantic-release bot 📦🚀