-
Notifications
You must be signed in to change notification settings - Fork 481
Simplify XMLHttpRequest polyfill #1151
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
Simplify XMLHttpRequest polyfill #1151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR simplifies the XMLHttpRequest polyfill for Node.js by removing the dependency on sync-request and relying solely on xmlhttprequest-ssl v4.0.0. The change addresses a bundling issue in Hyperlane caused by legacy CommonJS syntax in sync-request's transitive dependencies, which broke ESM-based builds.
Key Changes:
- Removed
sync-requestdependency from package.json - Upgraded
xmlhttprequest-sslfrom v3.1.0 to v4.0.0 - Simplified XMLHttpRequest polyfill to use
syncPolicy: "enabled"option instead of custom sync request handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk/src/polyfill/xmlhttprequest.ts | Replaced complex XMLHttpRequest class extension with simple factory function that enables synchronous requests via syncPolicy option |
| sdk/package.json | Removed sync-request dependency and upgraded xmlhttprequest-ssl to v4.0.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sdk/src/polyfill/xmlhttprequest.ts
Outdated
| } | ||
| } as any; | ||
| } | ||
| function XMLHttpRequest(opts?: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this refactored to no longer be a class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be a class? I believe this is working okay, and I think when I tried leaving it as a class it didn't work.
This might have to do with a change between xmlhttprequest-ssl 3.x and 4.x. In 3.x they were doing this:
https://github.com/mjwwit/node-XMLHttpRequest/blob/3.1.0/lib/XMLHttpRequest.js#L280
vs in 4.x:
https://github.com/mjwwit/node-XMLHttpRequest/blob/master/lib/XMLHttpRequest.js#L422
What is the issue with how it is currently done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not "wrong" per se, but... using functions as pseudo-classes is extremely old and outdated, it's not best practice and it's no longer recommended.
class is the modern standardized way to create classes, and class has various convenient features. So it's better to use it if we can.
This code should work...
globalThis.XMLHttpRequest = class extends $xmlhttprequest.XMLHttpRequest {};...or possibly even this code...
globalThis.XMLHttpRequest = $xmlhttprequest.XMLHttpRequest;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to using the class syntax.
Pauan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
|
@eranrund By the way... the SDK works natively on Node, so I'm curious why you needed to patch some things to make it work. What version of Node are you using? |
Please see the PR description - the stock SDK depends on It works fine on NodeJS but their build process failed due to the |
Yes, I know about that, but I'm referring to this:
We test with various build systems, all of them can handle Also, |
To be honest, I am not super familiar with their build so I am not sure why they ran into this and others haven't. |
|
Alright, understood. 👍 |
6bdab3e
into
ProvableHQ:feature/upgrade-xml-httprequest
Motivation
Hyperlane encountered a bundling issue with the Provable SDK due to its dependency on
sync-request. One ofsync-request’s transitive dependencies (sync-rpc) relies on legacy CommonJS require() syntax, which breaks Hyperlane’s ESM-based build:The SDK currently uses both:
xmlhttprequest-sslsync-requestto polyfill XMLHttpRequest in Node.js.
This dual approach exists because the version of
xmlhttprequest-sslpreviously in use incorrectly decoded synchronous responses as UTF-8, corrupting binary data. To work around this, synchronous requests were routed throughsync-request, which preserves raw bytes correctly.However, newer versions of
xmlhttprequest-sslnow handle response decoding properly, eliminating the need forsync-requestentirely.Note that sync requests are not desirable and long-term we should move away from that entirely, but that requires changes to snarkVM before the JS SDK can support that.
Test Plan
To verify this work I ran Hyperlane's test utility (https://github.com/troykessler/aleo-sdk-deploy) and was able to get it to work (with some minor changes).
I've also ran the aleo-multisig unit tests with this modified SDK and they passed.