chrisdickinson / raf

requestAnimationFrame polyfill library
MIT License
742 stars 54 forks source link

poor debugging experience in Chrome #26

Closed mattdesl closed 9 years ago

mattdesl commented 9 years ago

It seems like errors inside the raf handler are not really handled elegantly by Chrome.

require('raf')(function() {
    var foo = undefined;
    foo.bar()
})

Screen shot of debugger which breaks on errors. It doesn't give us any indication of where the error is actually coming from.

img

Using a normal requestAnimationFrame function works fine; Chrome can pinpoint the problem and break on it.

requestAnimationFrame(function() {
    var foo = undefined;
    foo.bar()
})
CMTegner commented 9 years ago

Hi!

You are absolutely right, this behaviour is a bit awkward. However, it isn't there without reason. Some background:

You're not completely at a loss, though. Evaluating e.stack at the breakpoint in your screenshot will give you the exact location of the actual exception.

mattdesl commented 9 years ago

e.stack does not support source maps in many browsers so this is fairly useless in modern workflows :(

@Raynos had mentioned that #21 is a "temporary workaround" until the spec is fixed. Boris says:

The exception should be reported as an uncaught exception, and is in at least Chrome and Firefox. The fact that the spec says to ignore it is a spec bug that needs fixing, I agree.

It looks like the spec has still not been fixed. Who can we bug to move this along? The current experience is painful enough that we are not able to use your module in development, even though we would like to. :smile:

Raynos commented 9 years ago

@mattdesl

e.stack does not support source maps in many browsers so this is fairly useless in modern workflows :(

Welcome to production. This is why you write javascript, so you can debug real production systems.

That being said, if exceptions are no longer swallowed in

Then it should be safe to remote the try catch.

The worst experience is swallowing exceptions in older browsers in production.

If these trade-offs do not meet your needs, feel free to write a different module. Although if @CMTegner thinks this module should not try/catch, I can write a different module for my needs as well.

mattdesl commented 9 years ago

Did those browsers ever swallow errors? It seems like it was only this polyfill swallowing errors (#20). Or are you just going on the safe side and assuming it's in a browser somewhere, since it was (regrettably) written into the spec?

I agree that this module should not allow exceptions to be swallowed. I've already forked it but would rather my modules just depend on this to reduce fragmentation and receive patches.

Raynos commented 9 years ago

@mattdesl the native raf implementation swallows exceptions.

Which is why, when the native raf is used, we pass a wrapper function that rethrows any exceptions in setTimeout in order to recover them, otherwise they would be sallowed.

Raynos commented 9 years ago

I only wrote the code because I wasted a day debugging a problem that was swallowed because I ran too much code within a raf loop. (I ran the entire rendering engine in a raf loop).

CMTegner commented 9 years ago

I'm on the fence with this one. On one hand I want this module to simply defer to the native raf when possible, but on the other hand I think that swallowing exceptions leads to an extremely poor experience for the developer (backed by the fact that it has been confirmed as a spec bug).

As long as the evergreen browsers don't report the exceptions as uncaught then I suggest leaving the try..catch in. If anyone can provide confirmation that the opposite is in fact true, then I'll revisit this.

mattdesl commented 9 years ago

Just tested using this export instead of the current code:

module.exports = function(fn) {
  return raf.call(global, fn)
}

Using throw 'MESSAGE' or foo.missing.blah() reports uncaught errors in the following browsers:

Chrome 41.0.2272.76 Safari 8.0 FF 36.0.1 IE 11

Do we know which browsers/versions were originally swallowing errors? Looking at the original issue that started the discussion, it seems like the polyfill (not the native requestAnimationFrame) was the one swallowing the errors.

Maybe we could include a test and have it run on SauceLabs/testling-ci to see if there are any errors swallowed on various browser versions.

CMTegner commented 9 years ago

I have a branch here which removes the try..catch and adds a test to ensure that the exception bubbles. Just need to figure out how I can provoke testling into testing a branch.

CMTegner commented 9 years ago

Looks like the test is passing in

all on OS X Yosemite. Don't know how older browsers will fare.

mattdesl commented 9 years ago

Leaving this here.

npm install @mattdesl/raf --save
CMTegner commented 9 years ago

I just pushed 3.0.0 to npm, removing the try..catch. I made it a major bump just to be on the safe side. Thanks for putting up with all the waiting @mattdesl.

mattdesl commented 9 years ago

Thanks @CMTegner !