Change quacksLike to check expected properties, not name

[dcousens]

Change quacksLike to check expected properties, not name

[treygriffith]

@dcousens If you're open to PRs, I can take a first pass at this.

[treygriffith]

Ok, I'm thinking through this a bit more, and I want to get your opinion before I proceed.

It seems the main use of this function is to determine whether value is constructed of Type. The way this is handled now is by checking whether the constructor.name of value is equivalent to the string type.

There are other ways to accomplish something similar, each with their own drawbacks:

1. Checking value instanceof Type.

Pros:

• fixes the mangling issue since we're checking the actual constructor

Cons:

• if Type is imported twice, this check may fail (an edge case, but difficult to debug)
• if value is constructed of an extension of Type, this check will fail
• if value exhibits the properties of Type but is not literally constructed, this check will fail

2. Checking Type.prototype.isPrototypeof(value)

Pros:

• fixes the mangling issue
• if value is constructed of an extension of Type this check will work
• if value is constructed by copying methods/properties from Type.protoype, but is not literally constructed, this check will work (e.g. Object.create)

Cons:

• if Type is imported twice, this check may fail (an edge case, but difficult to debug)
• if value exhibits the properties of Type but is not created, this check will fail (e.g. Object.assign)

3. Iterating over the methods/properties of Type.prototype to ensure equality with value

Pros:

• fixes the mangling issue
• if value is constructed of an extension of Type this check will work
• if value has copied methods/properties from Type.protoype, but is not literally constructed, this check will work (e.g. Object.create, Object.assign)

Cons:

• if Type is imported twice, this check may fail (an edge case, but difficult to debug)
• if value has methods/child objects that do the same thing functionally as Type, but are not literally copied, this check will fail

4. Iterating over the methods/properties of Type.prototype to ensure type equivalence with value

Pros:

• fixes the mangling issue
• if value is constructed of an extension of Type this check will work
• if value has copied methods/properties from Type.protoype, but is not literally constructed, this check will work (e.g. Object.create, Object.assign)
• if Type is imported twice, this check will work
• if value has methods/child objects that are of the same type but not literally copied, this check will work

Cons:

• Introduces a "false positive" possibility where value can "look like" Type by having functions with the same names, but not implementing those functions the same way.

My proposal would be to do 2 with a fallback to 4. What do you think @dcousens ? Is the false positive here acceptable, or are we better off rejecting values that are similar but not are not constructed of Object.created?

Additionally, is there a desire to extend this beyond constructors? If so, it may make sense to generalize this even further to have the quacksLike signature look like quacksLike(Foo.prototype), instead of quacksLike(Foo) (both in contrast to the current signature: quacksLike('Foo'))

[dcousens]

I can't see how to do this without a breaking change

[treygriffith]

This would definitely be useful. The current implementation breaks when you mangle constructor names, e.g. with Uglify.

What are you concerned with it breaking?

[dcousens]

It is a breaking change insofar as it indiscriminately allows any type if uglify is used.

In libraries that use this module, we recommend them to:

If you uglify the javascript, you must exclude the following variable names from being mangled: BigInteger, ECPair, Point. This is because of the function-name-duck-typing used in typeforce.

uglifyjs ... --mangle --reserved 'BigInteger,ECPair,Point'

[dcousens]

Apologies, @treygriffith I thought I was replying to #47

The breaking change here is that Users are currently passing a String, and we are changing the API to be an instantiated object with property matching.

[treygriffith]

@dcousens yeah I see that now. The way to introduce this might be through a new method (shame since the method name quacksLike is so good!).

Thanks for the note on the ECPair, Point, and BigInteger - I only found one of those myself, so will be good to preemptively add those others to the reserved list.

[dcousens]

@treygriffith major bump is how I would do it, tbh.

[dcousens]

@treygriffith I am open to it, I just can't guarantee when it will merge.
#45 is something I'd additionally want to solve by 2.0.0... so open to your thoughts there too.

[dcousens]

Can't respond right now, will tonight.

[dcousens]

if Type is imported twice, this check may fail (an edge case, but difficult to debug)

This issue is why quacksLike was introduced the way it is.

Introduces a "false positive" possibility where value can "look like" Type by having functions with the same names, but not implementing those functions the same way.

I think this is a great solution personally. It is by definition a great way to enforce quacksLike, better than focusing on the data too.
We could make it a score based system. 90% alike etc...

[treygriffith]

I kind of figured that might be the case. I'll re-work the PR to do a type equivalence check on all the properties.