search

ggrossetie / unxhr

Synchronous and asynchronous XMLHttpRequest for Node
MIT License
0 stars 3 forks source link

resolves #24: fix async binary transfer, fix sync content length #25

Closed djencks closed 3 years ago

djencks commented 3 years ago

Fixes async binary transfer, runs transfer tests both async and sync, and fixes a post content trimming mistake.

djencks commented 3 years ago

I added https tests and the ability to set rejectUnauthorized, with a test.... demonstrating a problem with sync errors.

ggrossetie commented 3 years ago

This looks great but could you please create one pull request per change otherwise it's hard to review:

Thanks

djencks commented 3 years ago

Separate pull requests would be a lot of work and don't reflect the history, which was that I made binary transfer work in a project, wrote some tests, and found some fairly egregious bugs which I fixed. Perhaps I could annotate the changes instead?

ggrossetie commented 3 years ago

Separate pull requests would be a lot of work and don't reflect the history,

The problem is that there's no history, all changes are already squashed into a single commit.

Perhaps I could annotate the changes instead?

Yes that could help. But it feels like you are fixing multiple things at once. So I would still like to have one commit/one pull request per fix/change.

ggrossetie commented 3 years ago

@djencks All your changes has been integrated in other pull requests. Thanks for your fine work :ok_hand: Could you please test against master to make sure that everything is working as expected?

Please note that I did a few changes to comply with the XMLHttpRequest.

ggrossetie commented 3 years ago

Closing, thanks again for all these fixes! :clap: