Skip to content
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

[ios][file-system] Fix copy async #27208

Merged
merged 4 commits into from
Feb 21, 2024
Merged

[ios][file-system] Fix copy async #27208

merged 4 commits into from
Feb 21, 2024

Conversation

alanjhughes
Copy link
Collaborator

Why

Closes #27021
When an image is selected from the photo library it will have a url like ph://***. Our URL conversion will accept this but when we access url.path it will be blank, causing the copy to fail.

How

Check if we are dealing with a photo library asset and handle the copy using the PHAssetResourceManager instead of the FileManager.

Test Plan

All tests still passing in bare-expo
Tested using the provided repro.

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Feb 21, 2024
@alanjhughes alanjhughes force-pushed the @alanhughes/filesystem/fix-copy branch from 7104035 to 6caa112 Compare February 21, 2024 15:16
@alanjhughes alanjhughes marked this pull request as ready for review February 21, 2024 15:56
Kudo
Kudo approved these changes Feb 21, 2024
View reviewed changes
packages/expo-file-system/ios/FileSystemHelpers.swift Outdated
import Photos

private let assetIdentifier = "ph://"
private let resourceManager = PHAssetResourceManager()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we lazy initiate ths instance or even just using local variable in the usage?

PHAssetResourceManager().writeData(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, we should make it lazy. We still need to keep it outside of the function because it has to live long enough for the callback to get executed.

@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 1 violation, 0 serious. See the review comments below for more insights.

Generated by ExpoBot 🤖 against 65514b1

expo-bot
expo-bot reviewed Feb 21, 2024
View reviewed changes
packages/expo-file-system/ios/FileSystemHelpers.swift

let firstResource = PHAssetResource.assetResources(for: asset).first
if let firstResource {
resourceManager.writeData(for: firstResource, toFile: toUrl, options: nil) { error in
Copy link
Collaborator

Choose a reason for hiding this comment

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

🟠 SwiftLint: Unused Closure Parameter Violation

Unused parameter in a closure should be replaced with _

@alanjhughes alanjhughes merged commit 7692d1c into main Feb 21, 2024
8 of 9 checks passed
@alanjhughes alanjhughes deleted the @alanhughes/filesystem/fix-copy branch February 21, 2024 21:05
aleqsio pushed a commit that referenced this pull request Feb 22, 2024
@alanjhughes @aleqsio
[ios][file-system] Fix copy async (#27208)
39a0197
alanjhughes added a commit that referenced this pull request Feb 23, 2024
@alanjhughes
[ios][file-system] Fix copy async (#27208)
38185ae
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Feb 27, 2024
@protetore
Copy link

protetore commented Feb 29, 2024 •
edited
Loading

Thanks for fixing this @alanjhughes !

@protetore
Copy link

protetore commented Feb 29, 2024 •
edited
Loading

@alanjhughes I gave it a try, but I'm facing another problem now: the files gets copied, but the promise never resolves.

Tried a simple test case of selecting a photo from camera roll and copying it to the documents path. The promise never resolves, but when a got to the directory and check if the file is there, it is.

await FileSystem.copyAsync({
  from: selectedPhoto,
  to: `${fullPath}/${pictureId}`,
});

console.log("You will never see this message");

@protetore protetore mentioned this pull request Feb 29, 2024
Closed
@alanjhughes
Copy link
Collaborator Author

Hi @protetore - This will be fixed by #27381. Apologies for the inconvenience. I shouldn't have missed that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@expo-bot expo-bot expo-bot left review comments

@Kudo Kudo Kudo approved these changes

@tsapeta tsapeta Awaiting requested review from tsapeta tsapeta is a code owner

Assignees
No one assigned
Labels
bot: fingerprint changed bot: suggestions ExpoBot has some suggestions 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.

[expo-file-system] copyAsync not working
5 participants
@alanjhughes @expo-bot @protetore @Kudo @brentvatne