Skip to content

Add blob support to fetch and expo-file-system/next #33152

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Dec 2, 2024
Merged

Add blob support to fetch and expo-file-system/next #33152

merged 21 commits into from
Dec 2, 2024

Conversation

aleqsio
Copy link
Contributor

@aleqsio aleqsio commented Nov 21, 2024 •
edited
Loading

Writing a more detailed description on this one – as @brentvatne mentioned support for blobs has been a topic that has been ongoing for a while so it justifies some of discussion.

Why

I'm working on the new filesystem package, which also includes network capabilities for file upload/download.
While we need dedicated functions for downloads, uploads, resumability, ect., I still feel that supporting web APIs is critical to ensure 3rd party libraries and general JS patterns work for what is ultimately a basic usecase.

In detail, this PR allows the following code to work:

import { File } from "expo-file-system/next";
import { fetch } from "@expo/fetch";

const src = new File(testDirectory, 'test.txt');
src.write('Hello world');

const formData = new FormData();
const blob = src.blob();

formData.append('data', blob);
await fetch('http://localhost:3000/upload', {
  method: 'POST',
  body: formData,
});

Directly passing a blob in also works:

const asset = await Asset.fromModule(require('../assets/qrcode_expo.jpg')).downloadAsync();
const src = new File(asset.localUri);

const blob = src.blob();

await fetch('http://localhost:3000/upload?key=TESTUSER', {
  method: 'POST',
  body: blob,
});

We can also consider moving blob to core, but since it's a pretty well defined interface I think it works if we just implement it – I don't think there's much to gain from passing the entire blob to native, instead we can pass a TypedArray.

Test Plan

Test suite tests. I've also tested them with a docker container that correctly saves an image file.

Checklist

@expo-bot
Copy link
Collaborator

expo-bot commented Nov 21, 2024 •
edited
Loading

The Pull Request introduced fingerprint changes against the base commit: 6e5ad8a

Fingerprint diff 
[
  {
    "op": "changed",
    "beforeSource": {
      "type": "dir",
      "filePath": "../../node_modules/expo",
      "reasons": [
        "rncoreAutolinkingAndroid",
        "rncoreAutolinkingIos"
      ],
      "hash": "ada6b5d2cba3410ae54d4f85d2689c3a3b1bb5df"
    },
    "afterSource": {
      "type": "dir",
      "filePath": "../../node_modules/expo",
      "reasons": [
        "rncoreAutolinkingAndroid",
        "rncoreAutolinkingIos"
      ],
      "hash": "b6c69888227b0d6a85184cbd39fb652469cd4b9c"
    }
  },
  {
    "op": "changed",
    "beforeSource": {
      "type": "dir",
      "filePath": "../../packages/expo",
      "reasons": [
        "expoAutolinkingIos",
        "expoAutolinkingAndroid",
        "expoConfigPlugins",
        "expoConfigPlugins"
      ],
      "hash": "6c65f85f5e738cfcb8164aa052bb105e6f559bef"
    },
    "afterSource": {
      "type": "dir",
      "filePath": "../../packages/expo",
      "reasons": [
        "expoAutolinkingIos",
        "expoAutolinkingAndroid",
        "expoConfigPlugins",
        "expoConfigPlugins"
      ],
      "hash": "0ea4158d71d27ee22601f55e5b8bd8d559bd9118"
    }
  },
  {
    "op": "changed",
    "beforeSource": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/android",
      "reasons": [
        "expoAutolinkingAndroid"
      ],
      "hash": "4d66a24190dbbed8cbeb6fbfb46ac97c36ad4be7"
    },
    "afterSource": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/android",
      "reasons": [
        "expoAutolinkingAndroid"
      ],
      "hash": "62bcd9d9f167b587fa977cd2f9e6ced468dcd513"
    }
  },
  {
    "op": "changed",
    "beforeSource": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/ios",
      "reasons": [
        "expoAutolinkingIos"
      ],
      "hash": "aa7b2b32eaf0555b0af606eaf229095d3be94423"
    },
    "afterSource": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/ios",
      "reasons": [
        "expoAutolinkingIos"
      ],
      "hash": "36ad27889e56b6530c5306f62fa4f10a6215f3af"
    }
  }
]

Generated by PR labeler 🤖

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Nov 25, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Nov 25, 2024
@aleqsio aleqsio marked this pull request as ready for review November 26, 2024 11:14
@github-actions GitHub Actions
Copy link
Contributor

Subscribed to pull request

File Patterns Mentions
packages/expo/** @ide, @tsapeta, @brentvatne
packages/expo-file-system/** @brentvatne, @Kudo, @ide, @tsapeta

Generated by CodeMention

@aleqsio aleqsio requested review from tsapeta, lukmccall and Kudo and removed request for tsapeta November 26, 2024 11:23
lukmccall
lukmccall approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this API; functions like .blob are pretty nice to use. Ideally, I prefer to avoid converting one object into another, but since we cannot do so here without changing the current API.

@aleqsio aleqsio requested a review from brentvatne November 26, 2024 14:21
@aleqsio aleqsio requested a review from ide November 26, 2024 14:21
Kudo
Kudo approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fantastic! didn't expect the api and implementation would be as clean as in this way. thanks for the great work.
i just left some nit comments and it overall looks great.

tsapeta
tsapeta approved these changes Nov 28, 2024
View reviewed changes
@Kudo Kudo mentioned this pull request Nov 29, 2024
Open
@aleqsio aleqsio merged commit dee32cc into main Dec 2, 2024
11 of 12 checks passed
@aleqsio aleqsio deleted the @aleqsio/add-blob-support-fs branch December 2, 2024 13:21
@aleqsio aleqsio mentioned this pull request Dec 2, 2024
Merged
3 tasks
entiendoNull pushed a commit to entiendoNull/expo that referenced this pull request Dec 13, 2024
@aleqsio @tsapeta @entiendoNull
Add blob support to fetch and expo-file-system/next (expo#33152)
e05faee 
Co-authored-by: Tomasz Sapeta <[email protected]>
Kudo pushed a commit that referenced this pull request Dec 16, 2024
@aleqsio @tsapeta @Kudo
Add blob support to fetch and expo-file-system/next (#33152)
043d3e1 
Co-authored-by: Tomasz Sapeta <[email protected]>
(cherry picked from commit dee32cc)
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kudo Kudo Kudo approved these changes

@tsapeta tsapeta tsapeta approved these changes

@lukmccall lukmccall lukmccall approved these changes

@brentvatne brentvatne Awaiting requested review from brentvatne

@ide ide Awaiting requested review from ide

Assignees
No one assigned
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aleqsio @expo-bot @Kudo @tsapeta @lukmccall @brentvatne