stevendesu / jsindex

1 stars 0 forks source link

Deep-equals fails on collection elements #20

Open stevendesu opened 5 years ago

stevendesu commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.