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.

Sign up for GitHub

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

Jump to bottom

refactor: remove lodash #593

Merged
merged 3 commits into from
Apr 1, 2024
Merged

refactor: remove lodash #593

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c489df
refactor: remove lodash
re-taro Mar 22, 2024
254b60e
refactor: impl same logic as lodash.isEqualWith
re-taro Mar 24, 2024
1695ddf
refactor: use lodash/isEqualWith for covering edge case
re-taro Mar 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 8 additions & 10 deletions src/to-have-form-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import escape from 'css.escape'
import {checkHtmlElement, getSingleElementValue} from './utils'
import {
checkHtmlElement,
getSingleElementValue,
isEqualWithArraysAsSets,
} from './utils'

// Returns the combined value of several elements that have the same name
// e.g. radio buttons or groups of checkboxes
Expand Down Expand Up @@ -63,15 +67,9 @@ export function toHaveFormValues(formElement, expectedValues) {
}
const formValues = getAllFormValues(formElement)
return {
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
}
}),
pass: Object.entries(expectedValues).every(([name, expectedValue]) =>
isEqualWithArraysAsSets(formValues[name], expectedValue),
),
message: () => {
const to = this.isNot ? 'not to' : 'to'
const matcher = `${this.isNot ? '.not' : ''}.toHaveFormValues`
Expand Down
11 changes: 7 additions & 4 deletions src/to-have-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import {checkHtmlElement, getMessage, getSingleElementValue} from './utils'
import {
checkHtmlElement,
getMessage,
getSingleElementValue,
isEqualWithArraysAsSets,
} from './utils'

export function toHaveValue(htmlElement, expectedValue) {
checkHtmlElement(htmlElement, toHaveValue, this)
Expand All @@ -24,9 +29,7 @@ export function toHaveValue(htmlElement, expectedValue) {

return {
pass: expectsValue
? Array.isArray(receivedValue) && Array.isArray(expectedValue)
? [...new Set(receivedValue)].every(v => new Set(expectedValue).has(v))
: receivedValue === expectedValue
? isEqualWithArraysAsSets(receivedValue, expectedValue)
: Boolean(receivedValue),
message: () => {
const to = this.isNot ? 'not to' : 'to'
Expand Down
9 changes: 9 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ function toSentence(
)
}

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!


export {
HtmlElementTypeError,
NodeTypeError,
Expand All @@ -233,4 +241,5 @@ export {
getTag,
getSingleElementValue,
toSentence,
isEqualWithArraysAsSets,
}