-
-
Couldn't load subscription status.
- Fork 12
convert anything to Buffer with promise #21
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
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.
Thanks for this PR. Feedback inline.
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.
Please don't bump the version in a PR. This should happen when the new version is actually published.
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.
sure
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 test removed? What happens when an invalid argument like an object is passed in?
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.
Response is able to accept many things, Typed arrays, ArrayBuffer, strings, Blob/files, FormData, URLSearchParams, ReadableStream, and even DataView and convert it to a ArrayBuffer using new Response(x).arrayBuffer()
It would be silly to just restrict it to only Blobs when you can accept many more types of stuff and let it be more loose and be used for more than just Blobs.
anything not understood by the Response constructor will be casted to a string using the toString function or secondly String(x)
so what happens when you do
await new Response({}).text()is that you get"[object Object]"await new Response(new Date()).text()yields"Sat Aug 24 2019 00:33:59 GMT+0200 (centraleuropeisk sommartid)"(since it has a toSting function)await new Response({toString() { return 'abc'}}).text()yields"abc"even doe it's a plain object
So it's a bit hard to get a error when Response can accept literally anything
So it's no longer just blob-to-buffer, it's now anything to buffer
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.
I would prefer this function to be named so it shows up with a name in stack traces. Also, I know that Response can handle many types, but the point of this package is to convert a blob, so the argument should be called blob.
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.
ok
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.
Can we keep this check in, but reverse the condition? It would be nice to help folks who are upgrading from 1.x and might leave the callback in by accident? It's similar to how we left the check in stream-to-blob when switching to promises: https://github.com/feross/stream-to-blob/blob/34d257b46d0f8c5d710194336d87f0623aa60fd4/index.js#L7
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.
mm, don't follow... 😕
maybe close this PR just and let you do as you want?
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.
Can you switch this example to use await?
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.
sure, i only used .then() as it was more cross platform compatible and many ppl just copy/paste code without using babel
@hubgit
hubgit
Oct 14, 2020
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.
Buffer.from won't work in a browser?
Responseworks in more browserblob.arrayBuffer()requires a polyfillchoose to use Response
Breaking change:
Use promise instead of callback
Bumped major version
closes #1