Skip to content

loose instanceof: check constructor function name instead #289

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

Merged
merged 2 commits into from
Oct 13, 2014

Conversation

weilu
Copy link
Contributor

@weilu weilu commented Oct 7, 2014

#287 fixed and rebased on top of #288. Tested minified version with testling (minified with browserify test/*.js | node_modules/uglify-js/bin/uglifyjs > test.js)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling eb4e888 on weilu:loose-instanceof into ca09849 on bitcoinjs:master.

@dcousens
Copy link
Contributor

dcousens commented Oct 8, 2014

Why not just use value.constructor.name === type.name?

@weilu
Copy link
Contributor Author

weilu commented Oct 8, 2014

@dcousens
Copy link
Contributor

dcousens commented Oct 8, 2014

Well, in IE, they'll both be undefined, so it won't fail... haha.

edit: I'd rather have an alternative path in there just for IE, then use a weird contorted way by default.

@dcousens
Copy link
Contributor

dcousens commented Oct 8, 2014

Also, you're using type.name already... how is that any better?

@weilu
Copy link
Contributor Author

weilu commented Oct 8, 2014

I'd rather have an alternative path in there just for IE, then use a weird contorted way by default.

Unfortunately the "weird contorted way" is the ES5 way.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling b55b10c on weilu:loose-instanceof into ca09849 on bitcoinjs:master.

dcousens added a commit that referenced this pull request Oct 13, 2014
loose instanceof: check constructor function name instead
@dcousens dcousens merged commit 7e897a5 into bitcoinjs:master Oct 13, 2014
@weilu weilu deleted the loose-instanceof branch October 13, 2014 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants