Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for plugins #25

Open
niemyjski opened this issue Jun 29, 2015 · 25 comments
Open

Add support for plugins #25

niemyjski opened this issue Jun 29, 2015 · 25 comments

Comments

@niemyjski
Copy link
Collaborator

We should try and modularize the code base so the jquery/ajax and window.onerror logic is separated out. So if you're not using it, you're not paying the overhead cost.

@blackrabbit99
Copy link

I forked you library to start the experiment
I'm gonna try do it for my project but still have several questions about how I should maintain it

@niemyjski
Copy link
Collaborator Author

What are your questions :)? I'll work with you until it's perfect and then we'll help maintain it too via a pull request :D

@blackrabbit99
Copy link

So lets look on simple example. jQuery error handling.

jQuery.error = function (message) {
    // Inject TraceKit
};

The question number 1. We should discuss error format. Maybe is there reason to describe error format in separate file? Because It should be the same for all plugins and handling options.

The second question. Where should the plugins store? In separate repos or into some folder?

@niemyjski
Copy link
Collaborator Author

@blackrabbit99 you could take a look at how raven did it. Also, there is an existing pull request here: https://github.com/occ/TraceKit/pull/49/files

I also have been talking to someone on Skype who wanted to work on this too. I'll forward this to them.

@niemyjski
Copy link
Collaborator Author

@blackrabbit99, I kinda like the idea of a plugins/integrations folder, what about you? We'll currently I think our window.onerror script would be a good example.

For example newer browsers return an actual error object but if not they specify like 5 parameters and we resolve it from there into a stack trace..

I'd like to have one generic method that takes the original error if it's there and the stack trace maybe? I'd want the original error always because we could parse extra properties off it (something we currently can't do). But in the event of where we don't have the error object, we'd have to parse the stack trace from the given meta data and only pass the stack. Thoughts on this?

@fizerkhan
Copy link

@niemyjski That will be great. i started with Jquery plugin https://github.com/MindscapeHQ/raygun4js/blob/master/src/raygun.tracekit.jquery.js But i ended up with lot of anonymous and file 0 and col 0 in stacktraces during ajax errors. Do you have any idea why it is happening?

@niemyjski
Copy link
Collaborator Author

I'm not sure, but if you could create a small unit test / sample that reproduces I'll help take a look. I'm going to spend a good portion of today trying and finishing up the unit tests

@fizerkhan
Copy link

@niemyjski Created jsfiddle for this test case. I added the external resource for Jquery, Tracekit and Tracekit Jquery pluign. Please check it

http://jsfiddle.net/fizerkhan/w64j03r9/10/

@niemyjski
Copy link
Collaborator Author

@fizerkhan if you look at the thrown error there is no line numbers or anything other than the function name. With this said, do you think it could be because jquery is loaded from a cdn and there is a security / cors functionality that is blocking the script/browser?

@niemyjski
Copy link
Collaborator Author

@fizerkhan So I did some looking and pulled jquery from rawgit as well (http://jsfiddle.net/bniemyjski/hrh0y8kd/3/). I think it's because we are trying to guess the stack trace.. If you look at the passed in error, it's just a string.

image

What are your thoughts on this?

@fizerkhan
Copy link

@niemyjski Thanks. I too tried with crossorigin="anonymous" in Jquery script tag as well as with local Jquery. I get the same null valued stacktraces.

I don't have any idea why Jquery returns error message as string.

I have a question:

Why Tracekit returns stack traces of null file name, line number, and column number for an error message which is just a string.

@niemyjski
Copy link
Collaborator Author

@fizerkhan I think it's because it's trying to compute it from the current stack trace, there maybe some issues with this code. We'll need to take a look into this and debug your sample more.

@fizerkhan
Copy link

@niemyjski Thanks, we will do it. I really appreciate your quicker response. 👍

@niemyjski
Copy link
Collaborator Author

No problem :), Thanks for your help!

@niemyjski
Copy link
Collaborator Author

I've been thinking about this more and want to do something with this shortly. I think I'd like to have plugins get registered and then they are called when events happens or on startup.

To me it makes sense that a plugin would automatically register handlers in two ways:

  1. It would register itself with TraceKit as a plugin
  2. It would wire itself up to any handlers (window.onerror) and report to tracekit when that happened.

My main goals is I want to have code that is very generic and platform independent. Currently we are doing some crazy stuff in the report method to try and resolve old ie stack traces. I think this may need to be deprecated (incomplete stack traces that get rethrown to the window.onerror) for the sake of making everything more robust.

Overall, I know that we will be breaking some backwards compatibility mostly with old versions of IE. However, that should be it. We can write async plugins support without promises that works on every browser.

@niemyjski
Copy link
Collaborator Author

On a last note, I don't think that we should be special casing anything. Everything should be going threw public functions if it needs to do something. This allows others to integrate with it and extend it.

@niemyjski
Copy link
Collaborator Author

I'm going to start on this this week in a feature/plugins branch. If anyone wants to have a discussion about this in the next three days please comment or send me an email and we can setup a google hang out or Skype session. I was planning on looking at the existing pull request as well as raven... I'd like to have a core library that's really trimmed down and and agnostic that just does the parsing and then the addin's add integration points of which to report errors from like handling promise errors, window on error etc..

@niemyjski
Copy link
Collaborator Author

Reference: https://github.com/occ/TraceKit/pull/49/files I'm not sure if we'd use this but it's a good look at what has been done and how it looks.

@niemyjski
Copy link
Collaborator Author

So I just spent the last two hours looking at the source and also looking at how I could do this with typescript.. There are two things that I need to figure out

  • Can I use es6 module syntax (targeting es3) and do a merge with plugins. You can do this with the legacy typescript external modules. Basically I want to be able to drop a plugin in and have it extend the TraceKit object... If I can't figure this out. I'm not going to use TypeScript or we need to come to a census
  • I found some even bigger issues with migrating to plugins and I almost feel like it's going to be more of a rewrite in some core areas.. For example. I want the core piece to just be parsing errors and then everything ontop of that is a plugin (noconflict, window.onerror stuff is in a window plugin). The things I'm seeing is the parsing methods are tied into looking up the source code via requests + other methods to try and parse out some method names.. This seems bad as it ties the resolve implementation to a browser.. I'd like to be able to tie into source maps and things like that.. So any feedback on how this should be done?

@niemyjski
Copy link
Collaborator Author

I'm working through a tracekit prototype, what do you guys think should be included in the core library? Did we just want to split out the jQuery stuff or also all code depending on global window object.

@niemyjski
Copy link
Collaborator Author

I finally got it to compile in typescript but the generated definitions are basically worthless. This is caused by the way report and computestack trace is a object and function. It's almost like I have to split those out into child classes which I don't want to do cause I want to keep the library small.

If we went down the route of typescript we'd have to create modules for each component. This wouldn't be a bad thing.. but it would greatly increase the size of the library as each small piece would need to be it's own module. This also would also cause issues with backwards combat.

@devinrhode2, @rwhogg, @csnover Could you leave your feedback on this.

With this said, my vote is to leave it as javascript as is, and move some small things like the async handlers to a plugin (if you include the it, it's wired up by default). We would leave the window.onerror stuff included as it's pretty deeply rooted into parsing/resolving function names etc ( 👎 ). Seems like this is the least amount of work without breaking everyone while saving some KB.

@rwhogg
Copy link
Contributor

rwhogg commented Oct 27, 2015

I personally have no strong feelings here - I'm much more interested in what @dcramer et. al think.

@niemyjski
Copy link
Collaborator Author

I started on this in the plugins branch in the main repo and also played around with this with @frankebersoll here https://github.com/frankebersoll/TraceKit/tree/feature/typescript

I think we are still trying to figure out the best way todo this with our breaking everything completely.

@benvinegar
Copy link
Contributor

For example. I want the core piece to just be parsing errors and then everything ontop of that is a plugin (noconflict, window.onerror stuff is in a window plugin).

👍

Let me know how I can help. We've vendored/forked TraceKit with a bunch of local modifications – like removing the setTimeout wrapping from TraceKit.js. I'd very much like to get back onto mainline TraceKit.

Migrating everything to TypeScript at the same time seems like a distraction, though.

@niemyjski
Copy link
Collaborator Author

@benvinegar That would be a huge help, I got really stuck on the typescript portion because of the way the new module system works, they don't make it easy. Is there any chance you'd take a stab at this (I'll create a plugins branch now). I'll work with you and help out over the holidays.

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

No branches or pull requests

5 participants