Skip to content
This repository has been archived by the owner on Jan 21, 2024. It is now read-only.

Add extended window.onerror support (#48). #49

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ssafejava
Copy link

This PR addresses #48.

Chrome Canary now supports sending a column number & stacktrace to window.onerror.

This patch enables TraceKit's window.onerror handler to use that stack trace if it exists. Additionally, it adds checks preventing the default setTimeout/setInterval wrapper from executing if this extended onerror arity is present.

Since this wrapping has been extended out to a plugin, I resurrected the old jQuery event/ajax wrappers as a plugin. Along with the async plugin, it now exists in /plugins.

The build was broken previously - this patch fixes it by disabling ADVANCED_OPTIMZATIONS. Building the project will result in a tracekit.min.js and tracekit.noplugins.min.js.

See the commit comments for more.

As of October this detection is much improved by simply creating an ErrorEvent object with a passed-in error, rather than throwing an error and checking (which never really worked well).

Chrome Canary now supports sending a column number & stacktrace to window.onerror.

This patch enables TraceKit's window.onerror handler to use that stack trace if it exists.
Additionally, it adds checks preventing the default setTimeout/setInterval wrapper from
executing if this extended onerror arity is present.

Since this wrapping has been extended out to a plugin, I resurrected the old jQuery event/ajax
wrappers as a plugin. Along with the async plugin, it now exists in /plugins.

Building the project will result in a tracekit.min.js and tracekit.noplugins.min.js.
Previously the closure compiler would build with ADVANCED_OPTIMIZATIONS, which causes
problems with exports. While traceKit could be rewritten to export all functions with string
syntax (e.g. `window['TraceKit'] = TraceKit;`), I think it is simpler and more future-proof
to just opt for simple optimizations instead.

I have also added a few additional tests to check extended window.onerror support as well as
build success. Run `grunt test` and it will open a browser window that will do a quick build test.
If, for example, ADVANCED_OPTIMIZATIONS is on, that build test will fail.
@devinrhode2
Copy link

Really awesome @ssafejava! We should get this merged into master.. I've been working on what I think the future of TraceKit should be, but at this point need to close some issues

@ssafejava
Copy link
Author

Thanks. Definitely would help, we get way more out of the extended window.onerror and it makes error reporting in modern browsers pretty nice. I've been using this in my app for the last month and it works well.

@JamesMGreene
Copy link

Why does this PR includes 2 new plugins? I think I understand what you mentioned about them in your PR description but they are still essentially independent from the important changes, which I'd also assume has something to do with why it hasn't been merged yet either.

I would recommend taking a slightly different approach here as well, since ErrorEvent is a separate spec item from the colNo (columnNumber) and error arguments to window.onerror. For example, some browsers currently support colNo but not error; some browsers support ErrorEvent but not the other spec changes.

TL;DR: These are independent spec changes in various levels of implementation cross-browser, so to maximize the benefit for current browsers, these all need independent feature tests.

Here is a JSFiddle demonstrating some of the more extensive feature testing I've done on the subject:
    http://jsfiddle.net/JamesMGreene/AVZqv/

@ssafejava
Copy link
Author

I had to make some significant build changes to make the plugin system work correctly - I couldn't see a way to integrate that without also fixing the problems in the build. The new dependencies are development only, this introduces no new runtime dependencies.

Regarding the more extensive feature testing, we could go down that route, but I had some trouble testing window.onerror specifically on startup (via caching old window.onerror, setting window.onerror, catching the error, silencing it, etc). I was running into intermittent conflicts with other libs, annoying conflicts with the chrome inspector (breaking on all uncaught exceptions would trigger on every load) and occasionally the error would show in the console.

Testing ErrorEvent was enough to get the feature detection working in edge Chrome which AFAIK is the only browser that fully supports sending an Error and colno object right now. Please correct me if I'm wrong and propose any changes to the detection that would help us catch this in other browsers.

@niemyjski
Copy link
Collaborator

@ssafejava , 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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants