Prop equal against circular structures

[BraulioVM]

Made this pull request to fix issue #411

The problem within this issue was that propEqual was using the utility function objectValues, which does not check against circular structures, and then compared the result of both objectValues with a normal QUnit.equiv. As this approach does not work, I decided to split QUnit.equiv into two separate functions:

• QUnit.equiv.deep: which performs the old QUnit.equiv computations
• QUnit.equiv.props: which compares object properties, ignoring their constructor. This function does check against circular structures and allows propEqual to work as it should. If this function is passed a non-object parameter, a QUnit.equiv.deep will be called.

I also created some tests for this new function (mostly copied them from deepEqual.js tests).

Fixes #411

[BraulioVM]

I have signed the CLA several times now, but don't know why it doesn't seem so to jquerybot

[jzaefferer]

Let's not add more exceptions. We should fix any issues in new files.

[BraulioVM]

I added test/propEqual.js to the exceptions because the tests in both files are almost the same

[jzaefferer]

Right, but we shouldn't duplicate crappy code.

[jzaefferer]

It looks like propEqual now replicates a lot of the deepEqual implementation. Maybe we're on the wrong track here. Maybe we should drop propEqual and remove the prototype check in deepEqual. That's what the io.js project did for their assert module: nodejs/node#621

[jzaefferer]

Not sure why the CLA check is failing, as far as I can tell your signature is fine. You can ignore that, I've pinged someone about that.

[BraulioVM]

As you said, I already removed objectValues and put ownObjectKeys in equiv.js. I also made QUnit.equiv backwards compatible. I didn't fix the formatting issues in test/propEqual.js as that could take a really long long time.

Talking about #776 (comment), I think that when comparing objects, what most people actually want is comparing like propEqual does, but can't really know for sure.

[jzaefferer]

Might as well drop innerEquiv and assign deepEquiv.props directly, then return that.

[BraulioVM]

I'm not sure that would totally make sense. How could propEquiv be a member of deepEquiv? They are kind of opposite. Something that could possibly be more natural would be to drop deepEquiv and assign innerEquiv and innerEquiv.props directly. The point of this is not having that "linguistic contradiction" with propEquiv being a subproperty of deepEquiv.

Does that make any sense?

[jzaefferer]

Sticking with innerEquiv is fine.

[jzaefferer]

@Krinkle @JamesMGreene @leobalter any input on the discussion about general changes to deepEqual (or propEqual)?

[leobalter]

I'm sorry I didn't give the proper attention to this PR, should we continue and update?

[jzaefferer]

I still think this is a valuable PR and we should try to pick it up and finish it. I just looked over the current stage again and while there's probably some minor details to fix (I noticed a missing end-of-file newline, so there are probably more style issues), overall this should be good to land.

[jzaefferer]

Before we put more time into this though, we should figure out what to do about #865. If we contribute our implementation to an external module and make use of that, we can (as easily) share code between propEqual and deepEqual anymore.

[leobalter]

does anyone want to continue the work on this? Should we close this at this point?

[trentmwillis]

I think this is still worth pursuing, but unless @BraulioVM wants to take this up again, it'd be best to close and open a new PR.

[trentmwillis]

It's been a couple more months, so I'm going to close this and hopefully we can address it in another PR.