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

Add support for NaN value #49

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Leomaradan
Copy link

NaN cannot be checked as equality. This commit add a test based on isNaN

nikolayg and others added 2 commits July 12, 2019 17:24
NaN cannot be checked as equality. This commit add a test based on isNaN
@coveralls
Copy link

coveralls commented Aug 15, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 1e9c938 on Leomaradan:master into 6296889 on mattphillips:master.

Copy link
Contributor

@anko anko left a comment

Choose a reason for hiding this comment

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

Just some thoughts.

@@ -173,5 +174,15 @@ describe('.diff', () => {
expect(diff(lhs, rhs)).toEqual({ b: 2 });
});
});

describe('nested NaN', () => {
test('returns empty object when there is nested NaN value', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test description confused me.

More concrete wording:

Suggested change
test('returns empty object when there is nested NaN value', () => {
test('treats NaN -> NaN as unchanged', () => {

@@ -3,6 +3,10 @@ import { isDate, isEmpty, isObject, properObject } from '../utils';
const diff = (lhs, rhs) => {
if (lhs === rhs) return {}; // equal return no diff

if (typeof lhs === "number" && typeof rhs === "number") {
if (isNaN(lhs) && isNaN(rhs)) return {}; // NaN is equal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Number.isNaN to check both Number type and NaN value simultaneously.

-   if (typeof lhs === "number" && typeof rhs === "number") {
-     if (isNaN(lhs) && isNaN(rhs)) return {}; // NaN is equal
-  }
+   if (Number.isNan(lhs) && Number.isNan(rhs)) return {}; // NaN is equal

Copy link

Choose a reason for hiding this comment

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

@anko you meant to cap the last 'n' --> Number.isNaN

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.

None yet

5 participants