-
Notifications
You must be signed in to change notification settings - Fork 62
Check for Blob instead of File #9
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
therealparmesh
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.
Thank you for the fix! Please see my review comments. 😄
| foo | ||
| ]) | ||
|
|
||
| t.true(formData.get('foo') instanceof File) |
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.
Shouldn't this be instanceof Blob? So we don't check for an instance of a sub-prototype?
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.
When you append a Blob instance to FormData and call FormData.get() you'll get an instance of File instead of Blob.
Try this in your console:
const foo = new Blob([], {})
const data = new FormData()
data.append('foo', foo)
data.get('foo') // <- will return a File instance|
On QuantumBA@13982a4 I'm doing duck-typing checking of |
|
@piranna I'm not sure I fully understand the difficulty you're experiencing. Shouldn't the solution be to |
|
Yes, this could be a solution, but this kind of things related to modifying commits history are considered bad practices on public git repos. Both |
|
I agree that |
|
Yeah, I use Anyway, I have just done a pull-request merging both histories and fixing them, and also with the |
|
@cretueusebiu I just pulled in #10 from @piranna. Let me know if this unblocks you! |
|
The file case worked fine before for me, because I was working in the browser. I use a |
|
|
|
I have just seen that ReactNative support Blob objects since July, and will have support for File objects in ReactNative 0.54 (currently in release candidate), so I'm not fully sure if the duck-typing has been a good idea, or it should be replaced for checking the actual objects, or do some checking for native support of the features on the system previously to use the duck-typing... What do you think? |
67f7ce5 to
6372773
Compare
14fe83c to
39b4f9b
Compare
|
Closing this because we are explicitly checking for Blobs. |
The
Fileclass extendsBlob, so this is useful when you create a Blob yourself (from the stream api for example).Note: When you append a
Blobinstance toFormDataand callFormData.get()you'll get an instance ofFileinstead ofBlob.That's why I added
t.true(formData.get('foo') instanceof File)in the new test.