-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixed issue where you could not register or unregister falsy items #83
Conversation
@@ -17,7 +17,11 @@ _.extend(TypeRegistry.prototype, { | |||
// found for the specified type, the default is | |||
// returned. | |||
get: function(type){ | |||
return this.registeredTypes[type] || this.registeredTypes['default']; | |||
if (this.registeredTypes.hasOwnProperty(type)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work in IE8
https://msdn.microsoft.com/en-us/library/ie/328kyd6z%28v=vs.94%29.aspx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why won't that work in IE8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh if registeredTypes is a host object it won't work :) but i guess registeredTypes will never be a host object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a regular object instantiated in line 8, I can test on IE8 (I am on Windows anyway... ducks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heheheh :) I just read a bunch about host objects, I think we are good 💃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works with no issues in IE7 mode of IE11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better safe than sorry right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what the drill sergeant always said during weapon training :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still would be good to use _.has
.
👍 |
@BorisKozo could you change this pull request to point to merge with the minor branch? Cheers |
Fixing issue #77
Added some unit tests for the type registry
Fixed 2 typos in existing unit tests that were found due to this change