Skip to content

Makeshift instanceof: check constructor function name instead #287

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

Closed
wants to merge 1 commit into from

Conversation

weilu
Copy link
Contributor

@weilu weilu commented Oct 5, 2014

Based on discussions in #255, #274, #231 and IRC, my conclusion is that proper duck typing seems too verbose and not checking types at all seems too dangerous. This is my attempt at finding a middle ground.

@dcousens dcousens changed the title loose instanceof: check constructor function name instead Lose instanceof: check constructor function name instead Oct 5, 2014
@dcousens dcousens changed the title Lose instanceof: check constructor function name instead Makeshift instanceof: check constructor function name instead Oct 5, 2014
@dcousens dcousens mentioned this pull request Oct 5, 2014
@jprichardson
Copy link
Member

@weilu Why the change of heart? Based upon our conversation, I'm not sure this is a good idea. Also, I got burned by this after minification... not sure why this was though.

@weilu
Copy link
Contributor Author

weilu commented Oct 5, 2014

@jprichardson because recently I've encountered a case where instanceof fails because npm dedup failed to resolve multiple copies of the same library. Good point about the minification. I will rebase on @dcousens' change and add tests for that. If it turns out to be an issue, let's close this and explore other options.

@weilu
Copy link
Contributor Author

weilu commented Oct 7, 2014

Close this as minification indeed cause problems

@weilu weilu closed this Oct 7, 2014
@weilu weilu deleted the loose-instanceof branch October 7, 2014 05:49
@dcousens
Copy link
Contributor

dcousens commented Oct 7, 2014

What were the problems?

@weilu
Copy link
Contributor Author

weilu commented Oct 7, 2014

edit: it was just an issue with the regex: making it non-greedy fixes it

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