search

wnr / element-resize-detector

Optimized cross-browser resize listener for elements.
MIT License
1.32k stars 118 forks source link

'object' strategy may throw error asynchronously #45

Closed adrianschmidt closed 8 years ago

adrianschmidt commented 8 years ago

If resize-detection using the 'object' strategy is triggered on an element that is then removed from the page, line 28 in src/detection-strategy/object.js might throw an error, and it seems this error is thrown from asynchronous code, which makes catching it somewhat difficult.

In our case, it made IE11 throw up a dialog asking the end-user to debug the application :(

If this error needs to be communicated to the consumer, it should be done using a promise rejection, or perhaps a callback.

https://github.com/wnr/element-resize-detector/blob/master/src/detection-strategy/object.js#L28

ie11_-_error_when_emptying_search_string_from_non-search_mode

wnr commented 8 years ago

I think it's weird that it throws that error even though the element has been detached from the DOM, that is not the desired behaviour. I'll investigate why it does so... In the mean time, could you perhaps provide a reduced example? Thanks.

adrianschmidt commented 8 years ago

I'll see what I can do :)

adrianschmidt commented 8 years ago

I've been looking at creating a reduced example, but honestly, I'm having some trouble with it. The original problem seemed to be caused by a race condition, because the number of elements made a difference, and it only showed up for some of my team, and not for the others. I've since been able to eliminate the problem in that specific case in our code, but it could still come occur anywhere else the detector is used.

It looks like we are using version 1.0.3 though, so I can at least see if I can replicate it with 1.1.0 or not.

Otherwise I think the easiest way is to see whether addListener is ever called from asynchronous code. It seems to me like it is, and if so, it should not be throwing errors, since they can't be caught in a reasonable manner.

I've tried reading the source myself, but haven't quite been able to follow it well enough to find the exact path from .listenTo to addListener where the error is thrown.

adrianschmidt commented 8 years ago

I can confirm that the problem persists in 1.1.0.

Here is a screenshot of the debugger in IE11, with a stacktrace:

debug with callstack for resize-detector

wnr commented 8 years ago

Hi, and thanks for the details.

The purpose of that error is to warn about broken invariants during development time. That error should never occur with proper usage, and therefore it should not be catched (since there is no way to recover from it). However, I do acknowledge that if such error persists in production code, one would like to disable/catch it. Perhaps it should not be a throw Error but instead a console.warn or similar that can be disabled at will. What do you think?

The actual error might appear if the element being listened to is being changed with el.innerHTML = or such, which removes the injected element. I guess that it could also appear (but I'm not sure) if one listens to an element and synchronously after detach it from the DOM. I need to verify this. Too bad that you could not get a reduced example to work.

As a side note: The object approach is being phased out in favor of the much faster (up to 32x!) scroll approach. We've been using the scroll approach for over a year in our large scale apps now, and it shows no flaws. So I recommend you switching to scroll unless you have very specific reasons :) I'll update the readme to reflect this.

Cheers

adrianschmidt commented 8 years ago

If the scroll method is stable, we'll definitely switch to that. Thanks!

wnr commented 8 years ago

Can this issue be closed? Thanks