-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Using Object.is implementation when compare values inside React.PropTypes.oneOf #6132
Using Object.is implementation when compare values inside React.PropTypes.oneOf #6132
Conversation
return x !== 0 || 1 / x === 1 / y; | ||
} else { | ||
// Step 6.a: NaN == NaN | ||
return x !== x && y !== y; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint emits warnings here
99:12 warning Comparing to itself is potentially pointless no-self-compare
99:23 warning Comparing to itself is potentially pointless no-self-compare
✖ 2 problems (0 errors, 2 warnings)
what should I do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just disable that warning for the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done and rebased
* inlined Object.is polyfill to avoid requiring consumers ship their own | ||
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is | ||
*/ | ||
function is(x, y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should move to a separate file at https://github.com/facebook/react/tree/master/src/shared/stubs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, just going to do this for now.
8229a5e
to
03925f4
Compare
@chicoxyzzy updated the pull request. |
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is | ||
*/ | ||
/*eslint-disable no-self-compare*/ | ||
function is(x, y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a weird place to define a polyfill. Seems like we should do something more analogous to https://github.com/facebook/react/blob/master/src/shared/stubs/Object.assign.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional and requested by me due to conversations had with @sebmarkbage a couple months ago, don't worry about it. We can deal with figuring out the polyfill story later.
Thanks! |
…ate_method Using Object.is implementation when compare values inside React.PropTypes.oneOf
implementation of #6131