Skip to content

[@mantine/core] Fix NaN handling in shallowEqual and useShallowEffect #7851

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

YukiKoNeko
Copy link
Contributor

Description

This pull request addresses an issue where useShallowEffect would incorrectly re-trigger when a dependency was NaN in both the previous and current renders. React's native useEffect hook treats NaN as equal to NaN for dependency comparison (following Object.is semantics), meaning the effect does not re-run in such cases. This change brings useShallowEffect's behavior in line with useEffect for NaN values.

The root cause was the shallowEqual utility function, which previously treated NaN as not equal to itself (NaN !== NaN is true under strict equality).

Changes Made

The primary modifications were made to the shallowEqual function to ensure NaN values are considered equal:

  1. Top-Level NaN Argument Handling:

    • Added a check at the beginning of shallowEqual to return true if both top-level arguments a and b are NaN.
      // After: if (a === b) { return true; }
      if (Number.isNaN(a) && Number.isNaN(b)) {
        return true;
      }
    • This ensures that shallowEqual(NaN, NaN) now correctly evaluates to true.
  2. Property Value NaN Handling:

    • Modified the comparison logic for object property values. If two property values a[key] and b[key] are not strictly equal (a[key] !== b[key]), an additional check is performed. If both are NaN, they are now considered equal for the purposes of shallowEqual.
      // Inside the loop for comparing object properties:
      if (a[key] !== b[key]) {
        // If not strictly equal, check if it's because both are NaN
        if (!(Number.isNaN(a[key]) && Number.isNaN(b[key]))) {
          return false; // Return false only if they are different and not both NaN
        }
      }
    • This ensures that objects like { value: NaN } and { value: NaN } are considered shallowly equal if their other properties also match.

These changes specifically target NaN comparison using Number.isNaN() and preserve the existing === strict equality behavior for all other value comparisons (e.g., 0 and -0 remain equal, as per original behavior). This approach avoids unintended side effects that a broader switch to Object.is() for all comparisons might introduce.

Impact

  • useShallowEffect and useShallowCompare: These hooks now correctly handle NaN values in dependency arrays. An effect managed by useShallowEffect will no longer re-run unnecessarily if a dependency changes from NaN to NaN.
  • Consistency with React: Aligns the behavior of useShallowEffect more closely with React's native useEffect concerning NaN dependencies.

This fix ensures more predictable and correct behavior for shallow comparison involving NaN values.

NaN -> NaN is now not considered a change anymore.
@YukiKoNeko
Copy link
Contributor Author

A test case would probably have to be written to ensure that this behavior does not re-appear. Also, I wasn't sure whether to fix it in shallowEqual directly or inside useShallowEffect but since all usages of shallowEqual are use cases that were affected by this behavior I changed shallowEqual.

@rtivital rtivital merged commit 704cbf7 into mantinedev:master May 19, 2025
1 check passed
@rtivital
Copy link
Member

Thanks!

rtivital pushed a commit that referenced this pull request May 19, 2025
…d correctly (#7851)

NaN -> NaN is now not considered a change anymore.
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