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

refactor: remove lodash #593

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Conversation

re-taro
Copy link
Contributor

@re-taro re-taro commented Mar 22, 2024

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

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 66 to 74
pass: Object.entries(expectedValues).every(([name, expectedValue]) => {
if (Array.isArray(formValues[name]) && Array.isArray(expectedValue)) {
return [...new Set(formValues[name])].every(v =>
new Set(expectedValue).has(v),
)
} else {
return formValues[name] === expectedValue
}
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 27 to 29
? Array.isArray(receivedValue) && Array.isArray(expectedValue)
? [...new Set(receivedValue)].every(v => new Set(expectedValue).has(v))
: receivedValue === expectedValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And reuse the isEqualWith function here.

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@re-taro
Copy link
Contributor Author

re-taro commented Mar 24, 2024

Good suggestion. I will revise it!

src/utils.js Outdated
Comment on lines 223 to 229
function isEqualWithArraysAsSets(arr1, arr2) {
if (Array.isArray(arr1) && Array.isArray(arr2)) {
return [...new Set(arr1)].every(v => new Set(arr2).has(v))
} else {
return arr1 === arr2
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@re-taro re-taro requested a review from gnapse April 1, 2024 10:44
@gnapse gnapse merged commit bd82f64 into testing-library:main Apr 1, 2024
5 checks passed
@gnapse gnapse mentioned this pull request Apr 1, 2024
@re-taro re-taro deleted the pr/remove_lodash branch April 8, 2024 13:20
Copy link

github-actions bot commented May 3, 2024

🎉 This PR is included in version 6.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants