feat: add blob support#634
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| // So we can only check the value is set (not null or undefined) | ||
| expect(testBlobValue).toBeTruthy(); |
There was a problem hiding this comment.
Then we need a way to check whether it is a Blob. You can try to create a blob with this data and see it has same content as you originally passed.
|
It looks like @deetergp is the CME for the linked issue, assigning him, and unassigning myself |
|
What type is returned from Onyx when we retrieve the Blob value? |
The type value that is returned by Onyx is object, because when we storing blob value into IndexedDB, it will only store all the blob values & properties, but it's no longer an instance of the Blob constructor See more at the last point: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#things_that_dont_work_with_structured_clone I think maybe the only we can check if the returned value is Blob or not by checking some properties in the returned object, but I am not sure if it's the best solution |
|
But then, how will we know that a collection in Onyx is of type Blob? We need something to differentiate, and we should return blobs from Onyx when we save Blobs. |
Ok..., please HOLD this one, give me some time I'll look into this more deeper 👀 about this one |
|
Feel free to raise this on slack to get confirmation and idea on how Blob support should be implemented. They might have better suggestions. |
Done, fixed So basically before, when we set a value in Onyx like this one: const blob = new Blob(["content"]);
Onyx.set("key", blob);
Onyx.get("key").then(res => console.log(res instanceof Blob)) // falseAnd we check if the value is instanceof blob it will return false, but if we do it like this, it will return true const blob = new Blob(["content"]);
Onyx.set("key", {
blobValue: blob
});
Onyx.get("key").then(res => console.log(res.blobValue instanceof Blob)) // trueIt's because when we set a value on Onyx, it will remove any null values if the Onyx value is typeof object and not an array Line 201 in a7c1666 Line 188 in a7c1666 Line 174 in a7c1666 react-native-onyx/lib/OnyxUtils.ts Lines 1156 to 1170 in a7c1666 react-native-onyx/lib/utils.ts Lines 108 to 115 in a7c1666 So when we set blob value, and then we check if it's instance of blob it will return false, because some properties might be removed during set item function process And also on the second example where we set the blob value like this, it returns true, it's because during the remove process, we only remove null values from the object itself, meaning it doesn't go to the child/grandchild properties, so when we check if it's instance of blob it will return true since there are no properties that are got removed Onyx.set("key", {
blobValue: blob
});cc: @parasharrajat |
|
That's interesting. I think we then need to make some changes to overall architecture to allow storing blobs. I do not want your research time to go in vain. We have a many expert contributors on slack which can suggest and improve your plan saving you time and bugs. I would suggest that you summarize the whole plan of storing the attachments on all platforms and post it on Slack so that we can get this reviewed. You can later mentioned all the steps that you have already taken and challenges along the way. This way we can optimize the solution and save us from increasing scope. |
|
I will be OOO this weekend 24 - 25 May, so I will back on 26 May |
|
@NJ-2020 is this needed? If not, let's close it. |
Details
Related Issues
$ Expensify/App#9402
Automated Tests
Unit tests were added to cover this fix.
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
9402_android_mweb.1.1.mp4
iOS: Native
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.-.2025-05-12.at.05.40.20.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-05-12.at.06.39.56.mov
MacOS: Desktop
Screen.Recording.2025-05-11.at.22.45.56.1.mov