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

Deep-equals fails on collection elements #20

Open
stevendesu opened this issue Nov 19, 2018 · 4 comments
Open

Deep-equals fails on collection elements #20

stevendesu opened this issue Nov 19, 2018 · 4 comments
Labels
bug Something isn't working MINOR This change adds new functionality, and will result in a change to the minor version

Comments

@stevendesu
Copy link
Owner

When writing unit tests, the following fails:

const collection = [{foo: "bar"}].index();
expect(collection[0]).toEqual({foo: "bar"});

The error returned says there are no "visual differences", but the values are different. This is likely because the collection element is a Proxy and I'm comparing it to an Object.

It may be possible to modify the Proxy in some way that this equality succeeds. It may be a matter of returning Object when they try to call get(obj, "__proto__"), or I may have to modify getOwnPropertyDescriptor or something. To be frank, I don't know what needs to be done (or if it's possible) -- but I'll look into this.

@stevendesu stevendesu added the bug Something isn't working label Nov 19, 2018
@stevendesu
Copy link
Owner Author

Looking into this, I actually found that my understanding was mistaken.

This test passes:

expect(collection[0]).toEqual({foo: "bar"});

This test fails:

expect(collection).toEqual([{foo: "bar"}]);

The problem is that collection has an idx property, and the array does not. So we just need to wrap the collection is its own Proxy so that get(obj, "idx") returns a value but has(obj, "idx") returns false

@stevendesu
Copy link
Owner Author

To make this slightly more complicated... You can't assign to this. So in the following:

const collection = [{foo: "bar"}];
collection.index();

The index() function can't replace collection with Proxy[collection, handler]. All it can do is return Proxy[collection, handler], meaning this can work:

let collection = [{foo: "bar"}];
collection = collection.index();

This isn't necessarily ideal as it changes out API, but it's also not that bad. My favorite short-hand would still work:

const collection = [{foo: "bar"}].index();

What may be a better option is to detach the idx property (and possibly the custom methods) from the collection. Then instead of collection.idx.age[28] we would need to say Array.getIndex(collection, "age", 28) or something similar. This also doesn't sound as fun, but it does decouple the indexing logic from the Array object (since I'm currently overloading the Array, which is considered bad practice)

Either way, there's some real thought that needs to be put into this

@stevendesu
Copy link
Owner Author

Alternate idea: What if I just add a .toArray() method?

const collection = [{foo: "bar"}].index();
expect(collection.toArray()).toEqual([{foo: "bar"}]);

I thought about this after re-reading my README and seeing this section:

Why do we override the array functions instead of using an observer?

...
A quick test in Chrome found that the native splice function was 300x faster than running splice through a Proxy like this for an array of 1 million elements.

Even if we could wrap the collection in a Proxy so that deepEquals returned true, the performance hit this would cause would not be worth it. So instead I should follow the same model I've used so far, and add a method to the collection.

@stevendesu stevendesu added the MINOR This change adds new functionality, and will result in a change to the minor version label Nov 19, 2018
@stevendesu
Copy link
Owner Author

If we find a way to solve this without adding a .toArray() method, we may need to make a major version change instead of a minor version change, depending on the ultimate solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working MINOR This change adds new functionality, and will result in a change to the minor version
Projects
None yet
Development

No branches or pull requests

1 participant