fix: proper fetching of binary data during get requests#11
fix: proper fetching of binary data during get requests#11mjwwit merged 6 commits intomjwwit:masterfrom
Conversation
|
This MR allows for synchronous and asynchronous GET requests that fetch binary data to be returned in a new field called XMLHttpRequest.response with the expected binary data. XMLHttpRequest.responseText is still text that is encoded in 'utf8' format. |
|
The original MR for this was wrriten by @georgestagg and was written to be merged into a forked version of this repo by @driverdan: driverdan@d251dae The reason I am re opening the MR here is because there were some merge conficts between this repo and the other forked version by @driverdan that did not seem like they were worth resolving. Also it doesn't seem like that forked repo is being maintained given the time of the last commit. |
|
Thanks for this! The inactivity of the original repository and other forks of it was the reasoning behind starting this one. As the package built from this repository is depended on by a large number of other packages I'll have to test it thoroughly, so please bear with me. |
|
@mjwwit Just wanted to let you know that I just added an extra commit dealing with local files containing binary data as well as support for |
mjwwit
left a comment
mjwwit
left a comment
There was a problem hiding this comment.
There's still some work to be done on this. Besides the comments there should also be a test file for this.
lib/XMLHttpRequest.js
Outdated
| } else { | ||
| self.status = 200; | ||
| self.responseText = data; | ||
| self.response = Buffer.from(fs.readFileSync(unescape(url.pathname), 'binary'), 'binary'); |
There was a problem hiding this comment.
I don't think this does what you want, binary encoding in Node.js is an alias for latin1. Also, first encoding the file buffer to binary only to then decode it again seems quite silly.
lib/XMLHttpRequest.js
Outdated
| } else { | ||
| try { | ||
| this.responseText = fs.readFileSync(unescape(url.pathname), 'utf8'); | ||
| this.response = Buffer.from(fs.readFileSync(unescape(url.pathname), 'binary'), 'binary'); |
There was a problem hiding this comment.
You're now effectively duplicating the work. I'd prefer a way where we can use the result of one readFileSync to provide both responseText and response with data.
lib/XMLHttpRequest.js
Outdated
|
|
||
| if (response && response.setEncoding) { | ||
| response.setEncoding("utf8"); | ||
| response.setEncoding('binary'); |
There was a problem hiding this comment.
Again, this is probably not what you think.
…eading from local files to only happen once in order to attain response and responseText
|
@mjwwit I've read your changes and made a commit in response. I did not know that setting binary to the Thanks and I look forward to your thoughts! |
…t to specify that it is testing text data
|
@mjwwit I also just commited a test file as well |
|
That is tons better! The only thing missing now is a reference to the new file (and the renamed one) in the package.json test script. If you add that I think we're good for a merge. |
|
@mjwwit Done! Thanks so much for taking the time to review this PR, some of the work we do involves downloading raw binary data so trying to find a fix for this repo was unfortunately very necessary since we do use it😆 |
|
I found a last problem in the code, but it's not (and was never) covered by any tests. As this is not really your problem I'll fix this myself after the merge. |
|
Thanks for your further work on this @RobertMirandola & @mjwwit and for getting this change merged. This will be very useful for me too and I look forward to being able to switch from my fork back to a proper npm package! |
|
I've released version 2.1.0 with your PR, my fix (and a test to verify) |
3 participants
No description provided.