-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
another fix for #261, in extendNative, share prototype, copy direct properties (not from prototype) #506
base: master
Are you sure you want to change the base?
Conversation
Note on testing: this does seem to work back to IE9, but not IE8 (which seems irretrievably broken at this point, and doesn't even support XMLHttpRequest.DONE anyways), and does work on modern Chrome, Firefox, and Safari. |
I don't think you need much help from me, this looks like great work. Thank you for catching and fixing my mistakes! There's currently work being done to find the best way for the community to manage this project, so I'll wait for that process to decide to merge and cut a release. |
Okay, it looks like @EatBreatheCode has been transferred ownership in issue #505 , but I don't know what else needs to happen in terms of release cycle and PR approvals. It seems we're probably in triage mode right now with the large number of open PR's and issues. |
Hi guys. I have already merged in quite a few PR's into my fork (which I will be integrating shortly). This week has been exceptionally busy at work so I likely won't be able to do much digging in until the weekend. All that being said @a2intl, I've heard only good things and have no doubt we'll be able to get your patch merged in as a priority. Also, always nice to meet another Zack @zackbloom! |
Hi @a2intl, take a peek at the latest code. Thanks! |
Just following up on this and whether you still think it should be merged. |
@CodeByZach , yes, I think this still should be merged, as I think the issues it fixes are still present. I remerged in master and got rid of the coffeescript original, but seemed to end up with quite a different minified version (I'm using minify v7.0.1 with no options, we should probably put the minification in the package.json). I'm also missing any place to put the tests (that were lazily added to tests/demo.html on the original commit e2d5dde#diff-62a0fac096c4ce34d9f89b48fc7c23683a4c0cc6da441053588bab55c4002323R47 ), do you want me to start an official |
Fixes for compatibility with other libraries that hook into
XMLHttpRequest
:share the prototype with the native (or at least the most-recently-installed)
window.XMLHttpRequest
.new XMLHttpRequest instanceof XMLHttpRequest
to be true, and code that expects functions such asXMLHttpRequest.prototype.send
to exist.copy the properties directly from
XMLHttpRequest
(DONE
and friends) and not from the prototype.undefined
(thetypeof from[key] isnt function
check should have beenfrom::[key]
, but we shouldn't have been looking at the prototype to start with.also minor fixes for not capturing the unneeded exception or return value of
extendNative
cc: @zackbloom as he wrote the original
extendNative
in d856e90 .This continues to not work for code that uses
XMLHttpRequest.prototype.open
rather thanreq.open
and expects it to be pace-hooked. The fix for that is to extendXMLHttpRequest.prototype.open
directly (a la Creating an XMLHttpRequest interceptor) rather than relying on a constructor-override and instance-property assign ofopen
, but that seems too drastic of a change for now. It would, however, be a much lighter touch on XMLHttpRequest and avoid having to override the constructor at all.