-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Implement Blob support for XMLHttpRequest #11573
Conversation
Would it be possible to revert the changes in the 2048 example to make this PR easier to review (less changed files)? Also does the 2048 example need changes? Probably doesn't do HTTP requests? |
cc @grabbou could you revert those changes please? |
ping @grabbou |
Fix import paths after rebasing Modify build targets
@@ -198,6 +198,7 @@ | |||
"serve-static": "^1.13.1", | |||
"shell-quote": "1.6.1", | |||
"stacktrace-parser": "^0.1.3", | |||
"uuid": "^3.0.1", |
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.
Is this necessary? uuid has been in use without this dependency (see https://github.com/facebook/react-native/pull/11573/files#diff-d7db9e5e49c3ea163f89547a0be6a438L19 in this PR).
This is one of the blockers, as adding the uuid package may conflict with another uuid dependency that we use internally at Facebook.
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.
This looks kind of bad, it probably only works because of some transitive dependency that also includes uuid.
Not sure what the best solutions would be, we could vendor the uuid module used internally or maybe add it to fbjs.
Another solution that could work is to use the uuid v4 import directly so require('uuid/v4')
(https://github.com/kelektiv/node-uuid#quickstart---commonjs-recommended)
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.
Per #16543 looks like the missing uuid
package is causing issues for some folks.
This is now in a good state internally and ready to land. Just need to get it looked at one more time by another Facebook employee. Once it's approved internally, I'll land it. Adding commits to the PR resets the diff to Needs Review internally so let's avoid doing that for now. We're so close! |
Thanks to everyone who did this! |
Woah! This is huuuge! 🔥🔥🔥 |
OMG it's merged! Amazing work @satya164 and reviewers! |
Thanks to everyone involved!! The PR actually has commits from 6 different persons + 6 additional reviewers! |
landed on RN53 or RN54? |
@sibelius you can check which releases a commit belongs to by looking at the tags on the commit itself. If you go to be56a3e, you can see it does not belong to any release yet (it's only tagged |
Summary: This PR is a followup to facebook/react-native#11417 and should be merged after that one is merged. 1. Add support for creating blobs from strings, not just other blobs 1. Add the `File` constructor which is a superset of `Blob` 1. Add the `FileReader` API which can be used to read blobs as strings or data url (base64) 1. Add support for uploading and downloading blobs via `XMLHttpRequest` and `fetch` 1. Add ability to download local files on Android so you can do `fetch(uri).then(res => res.blob())` to get a blob for a local file (iOS already supported this) 1. Clone the repo https://github.com/expo/react-native-blob-test 1. Change the `package.json` and update `react-native` dependency to point to this branch, then run `npm install` 1. Run the `server.js` file with `node server.js` 1. Open the `index.common.js` file and replace `localhost` with your computer's IP address 1. Start the packager with `yarn start` and run the app on your device If everything went well, all tests should pass, and you should see a screen like this: ! Pull to rerun all tests or tap on specific test to re-run it [GENERAL] [FEATURE] [Blob] - Implement blob support for XMLHttpRequest Closes facebook/react-native#11573 Reviewed By: shergin Differential Revision: D6082054 Pulled By: hramos fbshipit-source-id: cc9c174fdefdfaf6e5d9fd7b300120a01a50e8c1
Is there any documentation for using blobs? |
MDN is your friend |
Ok I missed that it was an implementation of the standard, I thought it was more like rn-fetch-blob. Some doc about it would probably help anyway. I'll submit a pull request if I can get it to work. |
@laurent22 did you figure it out? I'm still struggling with understanding how to create a Blob out of a local file Thanks |
I've never figured it out, so I've ended up using rn-fetch-blob |
@laurent22 just figured this out. It's a lot simpler that what I imagined. Check this out: https://stackoverflow.com/a/59002760/1658268 I hope this saves time from others looking to do the same thing. |
Summary: The eslint community config does not have the File and Blob polyfills in `globals` which have been part of the Javascript implementation for ~3 years. They were added to the Javascript API in #11573 by satya164. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [General] [Fixed] - Added File and Blob globals to eslint community config Pull Request resolved: #31293 Test Plan: Evidence these globals exist: 1. #11573 2. Executed the following: <img width="421" alt="Screen Shot 2021-04-02 at 12 08 50 PM" src="https://user-images.githubusercontent.com/3495974/113432946-466ae280-93ac-11eb-899c-3ca124e0af84.png"> <img width="317" alt="Screen Shot 2021-04-02 at 12 11 56 PM" src="https://user-images.githubusercontent.com/3495974/113433156-a82b4c80-93ac-11eb-99dc-0840d5ad9078.png"> 3. Receive in console: <img width="603" alt="Screen Shot 2021-04-02 at 12 09 59 PM" src="https://user-images.githubusercontent.com/3495974/113432996-5da9d000-93ac-11eb-81c6-88e6b059c733.png"> <img width="599" alt="Screen Shot 2021-04-02 at 12 12 27 PM" src="https://user-images.githubusercontent.com/3495974/113433174-b711ff00-93ac-11eb-8820-67039696f6ce.png"> Evidence the PR works: the globals in the PR identical to the others in the eslint community config that they are in Reviewed By: cipolleschi Differential Revision: D37214364 Pulled By: cortinico fbshipit-source-id: 71b9dec8d222a057c54f6cde6c6d8e85dd25f6f9
This PR is a followup to #11417 and should be merged after that one is merged.
What does the PR do?
File
constructor which is a superset ofBlob
FileReader
API which can be used to read blobs as strings or data url (base64)XMLHttpRequest
andfetch
fetch(uri).then(res => res.blob())
to get a blob for a local file (iOS already supported this)Test plan
package.json
and updatereact-native
dependency to point to this branch, then runnpm install
server.js
file withnode server.js
index.common.js
file and replacelocalhost
with your computer's IP addressyarn start
and run the app on your deviceIf everything went well, all tests should pass, and you should see a screen like this:
Pull to rerun all tests or tap on specific test to re-run it
Release Notes
[GENERAL] [FEATURE] [Blob] - Implement blob support for XMLHttpRequest