-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix bug in Record.equals when comparing against Map #1903
Conversation
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 great! Thank you for spotting and resolving this
src/Record.js
Outdated
this === other || (other && recordSeq(this).equals(recordSeq(other))) | ||
this === other || | ||
(other && | ||
this._keys && |
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._keys
should always be set because the instance doing the comparison is a Record
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.
Will fix
it('if compared against Map should return false', () => { | ||
const MyType = Record({ a: 1, b: 2 }); | ||
const t1 = MyType(); | ||
expect(t1.equals(Map({ a: 1, b: 2 }))).toBeFalsy(); |
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.
For completeness can you add a test which flips the operands to ensure the symmetric property?
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.
Sure
src/Record.js
Outdated
this === other || | ||
(other && | ||
this._keys && | ||
other._keys && |
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.
I wonder if the proper way to do this should be isRecord(other)
?
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.
It's definitely semantically more obvious what the check is doing but it depends on the guarantee that Record will always have _keys instantiated, which we will be alerted on if that is ever violated with this newly added unit test.
@@ -120,7 +120,8 @@ export class Record { | |||
|
|||
equals(other) { | |||
return ( | |||
this === other || (other && recordSeq(this).equals(recordSeq(other))) | |||
this === other || | |||
(isRecord(other) && recordSeq(this).equals(recordSeq(other))) |
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.
More correct and more clear. Nice!
This commit fixes a bug introduced by this change, which removed a check that '._keys' exists on the object being compared in Record.equals. This leads to a runtime uncaught error when a Record instance is compared against a Map instance.