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

Mimic React Hook's dependencies comparison #374

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

jezsung
Copy link
Contributor

@jezsung jezsung commented Jul 19, 2023

#155

This PR changes the comparison behavior to mimic the React Hook dependencies comparison which uses Object.is().

Previously, keys are compared by the == operator. The Dart == operator works differently from the JavaScript Object.is():

Object.is()

Object.is(NaN, NaN) // true
Object.is(0, -0) // false

Dart ==

double.nan == double.nan // false
0 == -0 // true

This PR checks if a list of keys contain a value with the type num and proceeds to handle cases where the value is either NaN or 0.0 and -0.0.

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (023fefc) 99.87% compared to head (897578f) 99.87%.

❗ Current head 897578f differs from pull request most recent head 941329d. Consider uploading reports for the commit 941329d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   99.87%   99.87%   -0.01%     
==========================================
  Files          18       18              
  Lines         804      781      -23     
==========================================
- Hits          803      780      -23     
  Misses          1        1              
Files Changed Coverage Δ
packages/flutter_hooks/lib/src/framework.dart 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -172,7 +172,23 @@ Calling them outside of build method leads to an unstable state and is therefore
if (!i1.moveNext() || !i2.moveNext()) {
return true;
}
if (i1.current != i2.current) {

Copy link
Owner

Choose a reason for hiding this comment

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

The docs need to be updated to match

@rrousselGit
Copy link
Owner

LGTM, with a tiny bit of documentation needed

Could you also add a changelog?

@jezsung
Copy link
Contributor Author

jezsung commented Jul 21, 2023

Documenting has been done! You might wanna resolve the CHANGELOG.md conflict. I'll just leave it for you.

@rrousselGit
Copy link
Owner

Looking good, thanks!

@rrousselGit rrousselGit merged commit 9bfd33a into rrousselGit:master Jul 25, 2023
2 checks passed
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