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

Already on GitHub? Sign in to your account

Drop jQuery support #35

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

In my opinion, this is too opinionated for something like TraceKit. TraceKit is a great library that provides functionality for other ligher levels libs to utilize.

With my recent commit of TraceKit.wrap(), that kind of paves the way for others to use it in a way to wrap anything they want. In the case for us (raven-js), I'm going to be providing a bunch of plugins to patch common libs that a user can pick and choose from. I'd rather not include source for jQuery for someone who isn't using jQuery.

I hope this makes sense. If anything, document the code that was removed and include it as a separate file so it still exists, but isn't so tightly coupled.

@mattrobenolt mattrobenolt Drop jQuery support
In my opinion, this is too opinionated for something like TraceKit. TraceKit is a great library that provides functionality for other ligher levels libs to utilize.

With my recent commit of TraceKit.wrap(), that kind of paves the way for others to use it in a way to wrap anything they want. In the case for us (raven-js), I'm going to be providing a bunch of plugins to patch common libs that a user can pick and choose from. I'd rather not include source for jQuery for someone who isn't using jQuery.

I hope this makes sense. If anything, document the code that was removed and include it as a separate file so it still exists, but isn't so tightly coupled.
487354f

In addition to intercepting setInterval/setTimeout, I think we should try intercepting all browser api's that accept a callback - event handlers, XMLHttpRequest .. and others I'm probably forgetting. I can't foresee any issues with this, so it's probably worth a shot

@devinrhode2, I completely agree. I think that if we're going to automatically patch things, we should only patch native Javascript stuff. We can leave library patches for a plugin or something of that sort.

My hope is that we can intercept all browser api's so that we don't have to patch libraries. I think it would be ok to stall this PR until we get something on that front, but I completely agree we should have no (or very little) code related specifically to jQuery. Maybe at most the library could by default hookup a window.$ if it's defined, maybe not.

Sounds fair to me. I think that should be the goal. TraceKit, by default, can/should patch native code. Let's leave the external libraries out of it. :)

Owner

occ commented Mar 3, 2013

For both #35 and #38,

After discussing this in more detail- I think that we should drop jQuery support (along with setTimeout/setInterval wrappers). IMO, All of these rather magical wrappers should actually be extracted out as plugins. This also simplifies the development as we wont't need to track a third party library.

The only remaining concern is functionality change. Essentially, this is a breaking change.
Since we don't (yet) have versions, we can't tell people that the code in master doesn't act the same way as it used to.

I'll tag this one as v0.1 and go on from there.

@occ occ added a commit that referenced this pull request Mar 3, 2013

@occ occ Merge #35
Remove jQuery support and update .gitignore
0d39401
Owner

occ commented Mar 3, 2013

Merged in 0d39401

@occ occ closed this Mar 3, 2013

Was this functionality moved to a plugin somewhere?

No not yet I believe
On Jul 22, 2013 8:00 PM, "Russell Davis" notifications@github.com wrote:

Was this functionality moved to a plugin somewhere?


Reply to this email directly or view it on GitHubhttps://github.com/occ/TraceKit/pull/35#issuecomment-21390467
.

It's a very very small amount of code for the record
On Jul 22, 2013 8:54 PM, devinrhode2@gmail.com wrote:

No not yet I believe
On Jul 22, 2013 8:00 PM, "Russell Davis" notifications@github.com wrote:

Was this functionality moved to a plugin somewhere?


Reply to this email directly or view it on GitHubhttps://github.com/occ/TraceKit/pull/35#issuecomment-21390467
.

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