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

fix(Storage): AL (asset library) methodology deprecated since iOS 8 #4054

Merged
merged 8 commits into from
Aug 15, 2020
Merged

fix(Storage): AL (asset library) methodology deprecated since iOS 8 #4054

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
366734c
fix(Storage): AL lib deprecated iOS 8 >
russellwheatley Aug 5, 2020
1e612fa
chore(Storage, iOS): imporve warning message
russellwheatley Aug 5, 2020
73fc529
Merge branch 'master' into @russell/storage-bug
russellwheatley Aug 5, 2020
f09eba4
Merge branch 'master' into @russell/storage-bug
russellwheatley Aug 6, 2020
1e33487
Merge branch 'master' into @russell/storage-bug
russellwheatley Aug 7, 2020
9454aea
Merge branch 'master' into @russell/storage-bug
Salakar Aug 13, 2020
423d2f1
Merge branch 'master' into @russell/storage-bug
mikehardy Aug 14, 2020
ea0f63e
Merge branch 'master' into @russell/storage-bug
mikehardy Aug 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/app/ios/RNFBApp/RNFBUtilsModule.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,16 @@ + (PHAsset *)fetchAssetForPath:(NSString *)localFilePath {
if ([localFilePath hasPrefix:@"assets-library://"] || [localFilePath hasPrefix:@"ph://"]) {
if ([localFilePath hasPrefix:@"assets-library://"]) {
NSURL *localFile = [[NSURL alloc] initWithString:localFilePath];
#if TARGET_OS_MACCATALYST
Copy link
Collaborator

Choose a reason for hiding this comment

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

@andymatuschak or @brunolemos - both of you are interested in this working on mac catalyst, can you confirm this change won't be a regression for you? We're trying to remove the AL APIs entirely and this is an intermediate step, but I'm not sure if the @available check is strong enough vs the #if TARGET_nnn test

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Mike, if the catalyst build passes that's good enough for my case. I don't think I use this specific feature related to this code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay - that's the hard part, I'm not sure it does - we don't have CI coverage of a catalyst build (yet) and none of us are currently targeting that platform personally, so it's untested as of yet. I have some action items related to that, but @brunolemos if you have any links to good docs on how to target react-native at catalyst that would help me cover it in CI to prove this works and keep it squared away for the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking. This'll be fine!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay great - thanks a ton for giving it a look @andymatuschak - I'll mark this for merge+release then, and of course if I/we are wrong about it I'll be listening and we'll get it fixed up so catalyst works. Cheers

static BOOL hasWarned = NO;
if (!hasWarned) {
NSLog(@"assets-library:// URLs are not supported in Catalyst-based targets; returning nil (future warnings will be suppressed)");
if (@available(macOS 11, *) || @available(iOS 12, *)) {
static BOOL hasWarned = NO;
if (!hasWarned) {
NSLog(@"'assets-library://' & 'ph://' URLs are not supported in Catalyst-based targets or iOS 12 and higher; returning nil (future warnings will be suppressed)");
hasWarned = YES;
}
asset = nil;
} else {
asset = [[PHAsset fetchAssetsWithALAssetURLs:@[localFile] options:nil] firstObject];
}
asset = nil;
#else
asset = [[PHAsset fetchAssetsWithALAssetURLs:@[localFile] options:nil] firstObject];
#endif
} else {
NSString *assetId = [localFilePath substringFromIndex:@"ph://".length];
asset = [[PHAsset fetchAssetsWithLocalIdentifiers:@[assetId] options:nil] firstObject];
Expand Down
6 changes: 6 additions & 0 deletions packages/storage/ios/RNFBStorage/RNFBStorageCommon.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ + (void)NSURLForLocalFilePath:(NSString *)localFilePath completion:(void (^)(
))completion {
if ([RNFBUtilsModule isRemoteAsset:localFilePath]) {
PHAsset *asset = [RNFBUtilsModule fetchAssetForPath:localFilePath];

if (!asset) {
completion(@[@"asset-library-removed", @"iOS 'asset-library://' & 'ph://' URLs have been removed, please provide the correct path to resource."], nil, nil);

return;
}
NSURL *temporaryFileUrl = [RNFBStorageCommon createTempFileUrl];
[RNFBStorageCommon downloadAsset:asset toURL:temporaryFileUrl completion:^(
NSArray *errorCodeMessageArray,
Expand Down