pace icon indicating copy to clipboard operation
pace copied to clipboard

another fix for #261, in extendNative, share prototype, copy direct properties (not from prototype)

Open a2intl opened this issue 3 years ago • 7 comments

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.

    • This fixes code that expects new XMLHttpRequest instanceof XMLHttpRequest to be true, and code that expects functions such as XMLHttpRequest.prototype.send to exist.
  • copy the properties directly from XMLHttpRequest (DONE and friends) and not from the prototype.

    • this also fixes the problem of function-valued properties of the prototype being copied as undefined (the typeof from[key] isnt function check should have been from::[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 d856e90643d381147f611d45f3bc7791a6f5122d .

This continues to not work for code that uses XMLHttpRequest.prototype.open rather than req.open and expects it to be pace-hooked. The fix for that is to extend XMLHttpRequest.prototype.open directly (a la Creating an XMLHttpRequest interceptor) rather than relying on a constructor-override and instance-property assign of open, 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.

a2intl avatar Nov 05 '20 19:11 a2intl

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.

a2intl avatar Nov 05 '20 19:11 a2intl

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.

zackbloom avatar Nov 06 '20 02:11 zackbloom

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.

a2intl avatar Nov 06 '20 04:11 a2intl

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!

CodeByZach avatar Nov 06 '20 05:11 CodeByZach

Hi @a2intl, take a peek at the latest code. Thanks!

CodeByZach avatar Nov 16 '20 00:11 CodeByZach

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.

Just following up on this and whether you still think it should be merged.

CodeByZach avatar Jan 21 '21 20:01 CodeByZach

@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 https://github.com/CodeByZach/pace/pull/506/commits/e2d5dde6c39f7922252d4530dbe3dbfc573810fc#diff-62a0fac096c4ce34d9f89b48fc7c23683a4c0cc6da441053588bab55c4002323R47 ), do you want me to start an official tests directory?

a2intl avatar Mar 10 '21 00:03 a2intl