Check types based on type.name equality in addition referential equality #51
Conversation
This is sometimes necessary if referential equality is broken, eg when types are proxied.
|
👍 Please merge, we are experiencing same problem on several teams in different environments. |
|
Can we get this merged please? |
|
I'll review this, and because it has a test, it's pretty much ready to merge. It would be nice to get a quick update to the docs referencing this change, if one of you has a minute. The change makes this either semver-patch or semver-major, depending on how it affects existing code. |
|
Cool! I'll have a look over the weekend for you On Fri, 30 Oct 2015, 22:24 Forrest L Norvell notifications@github.com
|
|
So - having a look at the docs - I don't think this warrants a change to documentation, it's just fixing a rare edge case where the strict equality test doesn't pass, in certain scenarios where types have got a bit messed up. It would be worth putting in a changelog, but you don't seem to have one going for this project. Could start one if you think it is necessary. This should be a semver-patch change, as it just is a bug fix that just makes things slightly more permissive. |
|
After taking a look at the docs, I agree (although at some point it would probably save somebody some time were we to document how nopt compares type equality, because there are so many subtly incompatible ways to do that). Landed as 0af9e18 and included in |
|
Woops, needed a little more fixup because of some insufficiently strict guards: 8900f8e. Landed as part of a quickly-released |
Credit: @wbecker PR-URL: npm/nopt#51
|
Nitpick: Only one of the .name checks in In all seriousness: Thanks for getting this merged and released, much appreciated. |
Check types based on type name as well as referential equality. Fixes: npm/nopt#48 Credit: @wbecker Reviewed-By: @othiym23 PR-URL: npm/nopt#51
This is sometimes necessary if referential equality is broken, eg when
types are proxied.
Fixes #48