Skip to content

Wrapping XMLHttpRequest#open() built in sensitive to order. #453

New issue

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

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

Already on GitHub? # to your account

Closed
mseeley opened this issue Jan 7, 2016 · 3 comments
Closed

Wrapping XMLHttpRequest#open() built in sensitive to order. #453

mseeley opened this issue Jan 7, 2016 · 3 comments
Assignees

Comments

@mseeley
Copy link
Contributor

mseeley commented Jan 7, 2016

Thanks for all the hard work you all do. Our team has enjoyed using Sentry and Raven.

I was finishing an upgrade to 1.3.0 and noticed a 2.0.1 release and took the plunge since we would be able to remove our own built in wrapper plugin.

Reading the Raven implementation it looks like you all are wrapping open(). You're probably aware this requires the consumer to define their event handlers before calling open() while the object's interface doesn't impose order for async requests.

jQuery for example calls open() before defining event handlers. It's not a jQuery-thing at the core, it's just super simple to miss out on Raven's wrapping due to flexibility of the XMLHttpRequest interface.

Our team opted to target send() and wrap the event handler asynchronously. This has seemed to handle any order of calling send(), open() and defining the event handlers. Of course it relies on the request being async.

var _send = global.XMLHttpRequest && global.XMLHttpRequest.prototype.send;
if (_send) {
  global.XMLHttpRequest.prototype.send = function () {
    var self = this;
    setTimeout(function () {
      if (self.readyState !== 4 && self.onreadystatechange) {
        self.onreadystatechange = Raven.wrap(self.onreadystatechange);
      }
    }, 0);
    return _send.apply(this, arguments);
  };
}

Regardless thanks again for the tool. Feel free to nod and close or run with it. Cheers!

@benvinegar
Copy link
Contributor

Reading the Raven implementation it looks like you all are wrapping open(). You're probably aware this requires the consumer to define their event handlers before calling open() while the object's interface doesn't impose order for async requests.

Yeah, you're right.

I wrapped send originally ... but I remember running into something that caused me to switch to open. For the life of me I can't remember what it was right now.

@benvinegar
Copy link
Contributor

Our team opted to target send() and wrap the event handler asynchronously. This has seemed to handle any order of calling send(), open() and defining the event handlers. Of course it relies on the request being async.

Ah, I think this might be related to what I was encountering. I believe onreadystatechange gets called as soon as you call send, synchronously, to indicate that the request has started, and this was causing me difficulties.

I'll definitely re-explore this pronto and see if we can get the right behavior in place for 2.1.0.

@benvinegar benvinegar self-assigned this Jan 7, 2016
@mseeley
Copy link
Contributor Author

mseeley commented Jan 7, 2016

Cheers Ben 👍 :)

benvinegar added a commit that referenced this issue Jan 12, 2016
Wrap XMLHttp.prototype.send instead of open (fixes #453)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants