Make it optional to wrap jQuery or async callbacks

[pieter]

We have our own wrapper for asynchronous callbacks and jquery events, where we do stuff beside exception handling.

TraceKit interferes with this by wrapping these threads too, even though we already catch exceptions in these async handlers ourselves.

TraceKits' wrapping should be optional, so it doesn't interfere with wrappers you create yourself.

This patch disables the wrapping of both setTimeout & friends and jQuery entirely. They can be re-enabled by calling TraceKit.wrapJQuery() or TraceKit.wrapAsyncCallbacks().

Refers to #35.

[mattrobenolt]

See #35

[pieter]

Yes, as you can see I also linked to that bug in my pull request. However, that issue doesn't address the wrapping of native async calls, which also shouldn't be wrapped unconditionally.

[mattrobenolt]

Whoa, I missed that. :) Sorry about that.

What's your opposition against wrapping natives, just out of curiosity?

[pieter]

We have our own wrapping of JQuery methods and async callbacks, which also use our custom error reporting and is used to run some code at the end of the run loop. TraceKit's methods get in the way of that, and it's unnecessary for us to have TraceKit wrap the methods since we already have the opportunity to wrap the necessary calls ourselves. We'd like to integrate TraceKit into our application with the least amount of impact, and all the global modificitions TraceKit does by default gets in the way of that.

[mattrobenolt]

I tend to agree. (I'm not a part of TraceKit, FWIW) That's my largest gripe at the moment that TraceKit is too opinionated about what it does. I'd much rather have it as a library and stays out of the way until I tell it to do something.

[devinrhode2]

What we really want here is to be able to override a default handler from TraceKit. (You guys have your own, but other people don't want to deal with extra stuff like telling it to wrap jQuery and native methods)

@pieter, do you think you could provide a code sample of what you do for the setInterval/setTimeout callbacks? The next version will use try{...}catch (e) { window.onuncaughtException(e) } code, so you can do whatever you want with what is currently TraceKit.report - would this by chance cover your use case?

We need a 'lean' version of TraceKit where all it does is synchronous normalization, and then there can be an opinionated full-featured one, with a configurable build like jQuery.

I think the simplest way to have a configurable build is simple inclusion/exclusion of files (like jQuery) .. I don't know how jQuery does this build thing, but I know with grunt we'll probably want to have the opening closure (function(window, undefined){ in the banner in grunt. If we don't need a intro.js and outro.js like jQuery, let's avoid it

[pieter]

@devinrhode2 we have a wrapping framework, which wraps jQuery callbacks, setTimeouts, Ajax callbacks, mutationobserver callbacks and some other stuff. This generic module is then used by our own error handling (which we don't want to get rid of yet) and some modules that need to perform checks at the end of each runloop. The order in which these are wrapped is important: for example, we want the error handling to be performed around the end-of-runloop code.

Inside our own error handler we try {} catch {} everything ourselves if it's not a development build, and then submit the captured exception to through Raven to TraceKit (Raven.captureException(e);). Then we submit it to our own error capturing code, and then show some UI feedback.

Basically, for our usecase, the only way we can use TraceKit is by passing on the exception ourselves. We won't be able to integrate it if it's going to add its own wrappers, since this would introduce too many changes.

[devinrhode2]

1. Could you give me a link to the framework you're talking about?

2. Would being able to override the wrappers be sufficient for your use case?

3. Below is some generic catch block code TraceKit will be using once it's re-written.

     var isFunction = function(obj){
       return Object.prototype.toString.call(obj) == '[object Function]';
     };
     //...
     } catch (e) {
       //probably window.onuncaughtException but maybe not. you can var over it
       if (typeof onuncaughtException !== 'undefined' && isFunction(onuncaughtException)) {
         onuncaughtException(e);
       } else {
         typeof console !== 'undefined' && console.warn && console.warn(
           'You should define a window.onuncaughtException handler for exceptions,' +
           ' or use a library like Sheild.js'
         );
         throw e;
       }
     }

Are there any drawbacks to this code that would require you to still add your own try/catch blocks around all callbacks? If so, what are those drawbacks? Please detail and perhaps include code

In the worst case scenario, you could no-op uncaughtException or make it undefined, and then the wrappers wouldn't do anything.

[pieter]

1. No, sorry, it's closed source ATM
2. I'd prefer there not to be any wrappers at all; that way there's no possibility of missing one when a new one is added, or issues in wrapping order when TraceKit would get loaded earlier / later than before
3. Yes, this would assume that TraceKit would wrap exactly the things that we want to wrap too. For example, TraceKit currently doesn't wrap MutationObserver callbacks. Using this would result in a mess of figuring out which callbacks TraceKit wraps, not wrapping those ourselves, and then wrapping the rest.

Another approach that would be fine for us is something like this:

• TraceKit doesn't install any handlers at all by default
• To use TraceKit, you need to call TraceKit.install() or similar, at which point callbacks get wrapped, global window handlers get installed etc.
• If you don't want to wrap callbacks, you can use TraceKit.install({ wrapCallbacks: false }) or similar instead. We could use this in our application, and just make sure that the exceptions are passed on to TraceKit ourselves, by calling something like TraceKit.handleException(e)

[devinrhode2]

2 I don't know what you mean by 'missing one'
2 (order) if tracekit comes before and wraps callbacks, and then you guys wrap them you'll just set window.onuncaughtException to anUndefinedValue, if tracekit comes after, then then you guys catch the error and tracekit doesn't even know about it.

3 I would love your help with a list of native functions TraceKit should wrap, so we can cover all the native ones.

If you want to help TraceKit by forking it and making a branch 'tracekit-lean' that would be extremely helpful. I'm thinking all this library should define is a TraceKit.normalize method

[devinrhode2]

I'm just trying to make sure we cover use cases for everyone. I'm pretty sure with the leaner version of TraceKit you won't have any problems.

[pieter]

Hey,

A lean version of TraceKit would be precisely what I'd need, and I think it could be useful for many more people. There can then be a more thorough version that installs wrappers etc. by default that could be used standalone, if you're just building a simple website.

I'm not sure if I'll have the time to work on that though, but I'll take a look.

[devinrhode2]

I have at least sunday here, but I'm working on a more feature-ful version, and I'm not sure there would even be that much overlap.

Disclaimer: I'm going to bed now so I won't be back online for at least 8-9 hours I'm thinking

[occ]

Can I close this now that #35 is merged?

[pieter]

Well, #35 addresses the jQuery support but not the setTimeout / setInterval. Might be better to open a new issue to extract that functionality into a separate plugin though. Perhaps you can take a look at how you want to implement that, since a modular system goes a bit further than what this pull request implements. If you're ok with that, we can close this one.

[ssafejava]

FYI this is moved into a /plugins folder in #49.

[russelldavis]

Looks like this issue has been addressed and should be closed.

[niemyjski]

@pieter , I'm helping manage the TraceKit project. Can you please create a pull request for this here https://github.com/csnover/TraceKit so we can get it merged in.

[niemyjski]

@pieter, this is now fixed in the v0.2.0 release here: https://github.com/csnover/TraceKit

[niemyjski]

I've made some changes around this in the root repo.