Skip to content

add arrayBuffer/Blob support to httpDo. implement loadBytes #2771

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

Merged
merged 6 commits into from
Apr 13, 2018

Conversation

Spongman
Copy link
Contributor

@Spongman Spongman commented Apr 9, 2018

re: #2693, closes #2674.

this PR adds arrayBuffer and Blob support to the httpDo method, and adds a convenience method loadBytes for loading a Uint8Array specifically.

adds some tests for loadBytes, including one for ensuring that the error callback is called when a 404 occurs.

@Spongman
Copy link
Contributor Author

Spongman commented Apr 9, 2018

ugh. this test only fails on travis. i can't tell why...

@Spongman
Copy link
Contributor Author

Spongman commented Apr 10, 2018

there seems to be an issue with the mocha test framework where an Error thrown in an inner Promise causes the test to fail rather than be caught in the outer Promise's catch clause. disabling this 404 test for now... also, this doesn't seem to happen on Windows, just on linux.

@limzykenneth
Copy link
Member

limzykenneth commented Apr 10, 2018

Looks great, I'll check it out later this week and maybe have a crack at the mocha problem as well, weird that it differs between windows and linux though...

@Spongman
Copy link
Contributor Author

yeah, i'm not really sure how to debug this, since it only happens within PhantomJS, not in the browser. I tried forcing the Promise polyfill in the browser, but it still didn't repro the problem.

@limzykenneth
Copy link
Member

Ok, I've tested all that I can think of and here are a few points:

Not sure why I didn't caught this before (maybe because I switched to Firefox recently) but if an error callback isn't provided, simply throwing the returned error will in Firefox cause it to fail silently while in Chrome and Safari cause it to throw an "Unhandled Promise Rejection" error. For some context of why see this question https://stackoverflow.com/questions/30715367/why-can-i-not-throw-inside-a-promise-catch-handler#30741722 The suggested method in the question using setTimeout gave a very unhelpful message so I wouldn't suggest that. I think maybe substituting throw err with console.error(err) could work but if you have a better solution do go for it.


Next is about the mocha problem which I think I figured out. It seems that webkit haven't implement finally for Promise yet which is why it failed in PhantomJS and not most browsers. If the test for that can be rewritten it would be great. Normally I use sinon.js to test that but it may not be worth to bring the whole thing in just for this test. You can decide how to proceed.


The rest may not be best for here as they are more general feature request, I can move it to a separate issue once this is merge.

  • It would be nice to be able to specify type for the httpDo overload that accept Request object otherwise anything we cannot auto detect its type will be parsed as text.

  • We probably need a new example for httpDo documentation that demonstrate its use without using the Request object as that may be a bit too high level for most not familiar with these standard.

Once the first two points are checked off this can be merged.

@Spongman
Copy link
Contributor Author

Fantastic! I'll switch to ’console.error’ and workaround the missing finally.

@limzykenneth limzykenneth merged commit 271d323 into processing:master Apr 13, 2018
@limzykenneth
Copy link
Member

Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementation of loadBytes
2 participants