Avoid deferring to the base type for object equality.#470
Avoid deferring to the base type for object equality.#470alexjeffburke wants to merge 1 commit intounexpectedjs:masterfrom alexjeffburke:fix/enumerablePropertyEdgeCaseInToEqualError
Conversation
Move checking object equality into a separate common function utils.checkObjectEqualityUsingType(). Use this within the base object type but also when comparing Error keys - since this function accepts the type, in the case of the Error type it ensures equality is evaluated using the subtype getKeys() thus addressing the mismatching keys issue.
|
This is another take on fixing #439: as compared to the other PR, this avoids the nastiness of calling into the super type |
| }); | ||
| }); | ||
|
|
||
| describe('when comparing Error objects with differing enumarable keys', () => { |
|
Hmm, I'm not sure I understand why calling |
|
Hmmm. It’s calling the parent |
|
I'd really like to get a fix in for the general case. This seems to work and cover all the cases we've discussed, but the |
|
Hmm. I’m not disagreeing about fixing the general case, but I think I like the idea of getting s fix out for Error ahead of that given node will fix that mishap at some point and I’d like to remove the hacks done in plugins asap. As for the general thing, I guess the reason I’m hesitant is I think we should be a little cautious papering over enumerability differences silently. If pushed, I think my personal preference would be to make it explicit as a job of getKeys() - in the case of Error that’s actually what we already do as the type adds message unconditionally. But note the type specified those semantics. |
|
@papandreou to follow up - if properties have been attached in a special way, isn’t it arguable that it has special meaning thus is better defined as a custom type? At least that roughly captures the debate I’m having internally. |
|
@alexjeffburke, for the record the linked commit also fixes the Error#message enumerability thing (I stole your test, too :). I’ve thought about it for a bit and couldn’t come up with any reason why papering over enumerability differences would be bad. If you really meant to compare two objects including the enumerability of the keys, I think you’d instinctively check the enumerability in a separate assertion anyway. It feels much too edge casey to be the default, even more so than a missing property vs. a present property with a value of |
|
I'll make up some time to look into this. |
|
@alexjeffburke I'm very sorry for being so late to the party. I agree with @papandreou that a general solution would be preferable and that papering over enumerability probably is fine. I think the spike @papandreou did looks interesting, but we need to make it faster with key indexes if possible. Otherwise I'm a bit worried that it will be too slow. |
|
Strike that @papandreou just informed me that there is an even better solution around :-) |
|
IMO this is fixed better by #482, OK to close this? |
|
This issue has been addressed by comprehensive enumerable property support that has now been merged to master. |
Move checking object equality into a separate common function
utils.checkObjectEqualityUsingType(). Use this within the base object
type but also when comparing Error keys - since this function accepts
the type, in the case of the Error type it ensures equality is
evaluated using the subtype getKeys() thus addressing the mismatching
keys issue.