#47 - be careful to not assume that the tooltip defined is, in fact, … #48
Conversation
|
I'm just seeing this sorry. Would a slightly more robust approach be to check if the constructor of |
|
I tend to avoid that in JS, though if my concern is outdated, let me know. In any other language, that's exactly what you'd do... See this question for more context http://stackoverflow.com/questions/7039260/can-i-use-constructor-name-to-detect-types-in-javascript |
|
I believe |
…he type of tooltip we are expecting
|
Not sure what to do about the failing tests; the fail for me locally as well but can't imagine my change is responsible for a sass fail! Any thoughts? |
| @@ -62,7 +63,7 @@ export default Ember.Mixin.create({ | |||
| destroyTooltip: on('willDestroyElement', function() { | |||
| const tooltip = this.get('tooltip'); | |||
|
|
|||
| if (tooltip && Ember.typeOf(tooltip) === 'object') { | |||
| if (tooltip && tooltip instanceof Tooltip) { | |||
There was a problem hiding this comment.
@sir-dunxalot - take a look - I've updated this pull to do the instanceof check you suggested. It's worth noting that it looks like another attempt at fixing this bug was merged here 432a943#diff-6d875f075dad177398e49011cc51f439R65 but that is simply not a good enough check; the collision in my case is w/ another object. (For others who might be reading this, https://github.com/Glavin001/ember-c3 uses a property tooltips - (a hash, therefore also an object) for it's graphs, so just by adding this tooltip library, any time you navigate away from a graph, your app crashes since this library is added to every component and hooks into the willDestroyElement).
I've tested this w/ my app and it works for me, would love very much to get a release of this!
There was a problem hiding this comment.
Great, thanks for the update. I'll get on this ASAP so we can make another release - likely sometime this week.
…the type of tooltip we are expecting