[expo-file-system][next] Add download function. #30841

aleqsio · 2024-08-06T16:32:25Z

Why

We allow downloading files and immediately getting a File instance to read the downloaded file.

How

Added a module-level download function. We accept both target files and directories (we try to get the filename from correct headers)

Test Plan

Added test cases.

Checklist

Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
Conforms with the Documentation Writing Style Guide
This diff will work correctly for npx expo prebuild & EAS Build (eg: updated a module plugin).

expo-bot · 2024-08-06T16:35:01Z

The Pull Request introduced fingerprint changes against the base commit: 9bba2fe

Fingerprint diff

[
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/android",
      "reasons": [
        "expoAutolinkingAndroid"
      ],
      "hash": "d82cffb9b98d8fa3d17b1504eb3cf5bff7d9e516"
    }
  },
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-file-system/ios",
      "reasons": [
        "expoAutolinkingIos"
      ],
      "hash": "92f1b0f73ad303ed8a7c63a0e341b735ef87a8cf"
    }
  },
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-modules-core",
      "reasons": [
        "expoAutolinkingIos",
        "expoAutolinkingAndroid"
      ],
      "hash": "a4c3c84f7eff58977cc1662c607eea45b6ba7f8f"
    }
  }
]

Generated by PR labeler 🤖

aleqsio · 2024-08-22T13:29:42Z

packages/expo-file-system/ios/Next/FileSystemNextExceptions.swift

@@ -12,3 +14,9 @@ internal class MoveDirectoryToFileException: Exception {
    "Unable to move a directory to a file"
  }
 }
+
+internal class UnableToDownloadException: GenericException<String> {


Is this OK, or should we make several reasons (no response, invalid response code (300/400), other)?

We could consider that in the future, but for now I think we don't have a very good mechanism for handling exceptions on the JS side – we should have our own Error class that can contain some custom data (like the response code) etc. and works well with error chaining

.../expo-file-system/android/src/main/java/expo/modules/filesystem/next/FileSystemNextModule.kt

lukmccall

Android part looks good!

...ges/expo-modules-core/android/src/main/java/expo/modules/kotlin/devtools/OkHttpExtensions.kt

...o-file-system/android/src/main/java/expo/modules/filesystem/next/FileSystemNextExceptions.kt

tsapeta · 2024-08-22T21:10:59Z

packages/expo-file-system/ios/Next/FileSystemNextModule.swift

@@ -6,6 +6,40 @@ public final class FileSystemNextModule: Module {
  public func definition() -> ModuleDefinition {
    Name("FileSystemNext")

+    AsyncFunction("download") { (url: URL, to: FileSystemPath, promise: Promise) in
+      let downloadTask = URLSession.shared.downloadTask(with: url) { urlOrNil, responseOrNil, errorOrNil in


Would be great if there is an async/await version of that function

This is not resolved 😅 But I think it might be hard to make an async version of downloadTask(with:) as opposed to the already existing download(from:). The latter doesn't seem to support downloading when the app is suspended though.

packages/expo-file-system/src/next/index.ts

tsapeta · 2024-08-22T21:21:21Z

apps/test-suite/tests/FileSystemNext.ts

+        const directory = new Directory(testDirectory);
+        const output = await download(url, directory);
+        const file = new File(
+          testDirectory + (Platform.OS === 'android' ? 'jpeg.jpg' : 'jpeg.jpeg')


Can't we use the same extension on all platforms?

We rely on OS functions to get a filename based on URL headers – I don't want to write my own now, but can look into that a bit more in the future.
https://developer.android.com/reference/android/webkit/URLUtil#guessFileName(java.lang.String,%20java.lang.String,%20java.lang.String)

tsapeta · 2024-08-29T10:52:11Z

packages/expo-file-system/ios/Next/FileSystemNextExceptions.swift

@@ -12,3 +14,9 @@ internal class MoveDirectoryToFileException: Exception {
    "Unable to move a directory to a file"
  }
 }
+
+internal class UnableToDownloadException: GenericException<String> {


We could consider that in the future, but for now I think we don't have a very good mechanism for handling exceptions on the JS side – we should have our own Error class that can contain some custom data (like the response code) etc. and works well with error chaining

tsapeta · 2024-08-29T10:58:41Z

packages/expo-file-system/ios/Next/FileSystemNextModule.swift

@@ -6,6 +6,40 @@ public final class FileSystemNextModule: Module {
  public func definition() -> ModuleDefinition {
    Name("FileSystemNext")

+    AsyncFunction("download") { (url: URL, to: FileSystemPath, promise: Promise) in
+      let downloadTask = URLSession.shared.downloadTask(with: url) { urlOrNil, responseOrNil, errorOrNil in


This is not resolved 😅 But I think it might be hard to make an async version of downloadTask(with:) as opposed to the already existing download(from:). The latter doesn't seem to support downloading when the app is suspended though.

tsapeta · 2024-08-29T10:59:40Z

packages/expo-file-system/ios/Next/FileSystemNextModule.swift

@@ -6,6 +6,40 @@ public final class FileSystemNextModule: Module {
  public func definition() -> ModuleDefinition {
    Name("FileSystemNext")

+    AsyncFunction("download") { (url: URL, to: FileSystemPath, promise: Promise) in


To be honest I think this should return a shared object (like the download task) that you can then pause/resume/cancel/listen to progress. It might be better to call it downloadFile(Async?) (since it returns a File instance) and then we can introduce more powerful download/downloadTask in the future.

packages/expo-file-system/ios/Next/FileSystemNextModule.swift

Co-authored-by: Expo Bot <[email protected]>

…ilesystem/next/FileSystemNextExceptions.kt Co-authored-by: Łukasz Kosmaty <[email protected]>

…kotlin/devtools/OkHttpExtensions.kt Co-authored-by: Łukasz Kosmaty <[email protected]>

Co-authored-by: Expo Bot <[email protected]>

# Why We allow downloading files and immediately getting a `File` instance to read the downloaded file. # How Added a module-level `download` function. We accept both target files and directories (we try to get the filename from correct headers) # Test Plan Added test cases. # Checklist  - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). --------- Co-authored-by: Expo Bot <[email protected]> Co-authored-by: Łukasz Kosmaty <[email protected]>

expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 6, 2024

github-actions bot added the bot: fingerprint changed label Aug 6, 2024

aleqsio force-pushed the @aleqsio/add-download branch from 2b40b10 to a23ffff Compare August 22, 2024 12:45

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 22, 2024

aleqsio marked this pull request as ready for review August 22, 2024 13:27

aleqsio requested review from brentvatne, Kudo, ide and tsapeta as code owners August 22, 2024 13:27

aleqsio requested a review from lukmccall August 22, 2024 13:28

aleqsio commented Aug 22, 2024

View reviewed changes

lukmccall reviewed Aug 22, 2024

View reviewed changes

.../expo-file-system/android/src/main/java/expo/modules/filesystem/next/FileSystemNextModule.kt Outdated Show resolved Hide resolved

aleqsio requested a review from lukmccall August 22, 2024 14:02

expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Aug 22, 2024

lukmccall approved these changes Aug 22, 2024

View reviewed changes

...ges/expo-modules-core/android/src/main/java/expo/modules/kotlin/devtools/OkHttpExtensions.kt Outdated Show resolved Hide resolved

...o-file-system/android/src/main/java/expo/modules/filesystem/next/FileSystemNextExceptions.kt Outdated Show resolved Hide resolved

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 22, 2024

tsapeta reviewed Aug 22, 2024

View reviewed changes

aleqsio requested a review from tsapeta August 23, 2024 16:34

aleqsio force-pushed the @aleqsio/add-download branch from 8b2d4a5 to a64b846 Compare August 29, 2024 10:05

tsapeta reviewed Aug 29, 2024

View reviewed changes

tsapeta approved these changes Aug 29, 2024

View reviewed changes

packages/expo-file-system/ios/Next/FileSystemNextModule.swift Show resolved Hide resolved

aleqsio and others added 5 commits August 29, 2024 18:16

Add file downloads

f5d8cb3

Fix lint

9d89e3b

Remove comment

4a464b8

Add changelog

0eea8fd

Update packages/expo-file-system/CHANGELOG.md

8583951

Co-authored-by: Expo Bot <[email protected]>

aleqsio and others added 14 commits August 29, 2024 18:16

Shorten function

ba71849

Remove unused testcases

cec43e1

Remove comment

97f153a

Move to okhttp

8f06c0a

Don't use blocking okhttp

5a0188b

Remove unused line from exceptions

826d988

Update packages/expo-file-system/android/src/main/java/expo/modules/f…

07c0bbf

…ilesystem/next/FileSystemNextExceptions.kt Co-authored-by: Łukasz Kosmaty <[email protected]>

Update packages/expo-modules-core/android/src/main/java/expo/modules/…

0f9c3ca

…kotlin/devtools/OkHttpExtensions.kt Co-authored-by: Łukasz Kosmaty <[email protected]>

Update changelog

beeba2a

Update packages/expo-modules-core/CHANGELOG.md

24e96c0

Co-authored-by: Expo Bot <[email protected]>

Move to a static function

470a1ff

Remove types

eaa0772

Rebuild

adc8e64

Rename function

3401835

aleqsio force-pushed the @aleqsio/add-download branch from 9276c7e to 3401835 Compare August 29, 2024 16:17

aleqsio merged commit bab7aec into main Aug 30, 2024
12 of 13 checks passed

aleqsio deleted the @aleqsio/add-download branch August 30, 2024 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[expo-file-system][next] Add download function. #30841

[expo-file-system][next] Add download function. #30841

aleqsio commented Aug 6, 2024 •

edited

Loading

expo-bot commented Aug 6, 2024 •

edited

Loading

aleqsio Aug 22, 2024

tsapeta Aug 29, 2024

lukmccall left a comment

tsapeta Aug 22, 2024

tsapeta Aug 29, 2024

tsapeta Aug 22, 2024

aleqsio Aug 23, 2024 •

edited

Loading

tsapeta Aug 29, 2024

tsapeta Aug 29, 2024

tsapeta Aug 29, 2024

[expo-file-system][next] Add download function. #30841

[expo-file-system][next] Add download function. #30841

Conversation

aleqsio commented Aug 6, 2024 • edited Loading

Why

How

Test Plan

Checklist

expo-bot commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukmccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleqsio Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleqsio commented Aug 6, 2024 •

edited

Loading

expo-bot commented Aug 6, 2024 •

edited

Loading

aleqsio Aug 23, 2024 •

edited

Loading