fix(Storage): AL (asset library) methodology deprecated since iOS 8 #4054
Conversation
russellwheatley
changed the title
|
not reviewing (per request not to) but this also caused a build failure on one of the other os as well (catalyst I think?) because it's so dead-dead :-) |
|
@mikehardy Yeah, I took a look at that issue when making this PR, it partly affects the same area of code! |
| @@ -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 | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for checking. This'll be fine!
There was a problem hiding this comment.
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
|
I'm inclined to merge this but I asked for a targetted review from the 2 known-interested catalyst folks. If I don't hear back I'll merge but hopefully one or both gives a thumbs up :-) |
mikehardy
added
Workflow: Needs Review
mikehardy
removed
Workflow: Needs Review
…nvertase#4054) * fix(Storage): AL lib deprecated iOS 8 > * chore(Storage, iOS): improve warning message Co-authored-by: Mike Diarmid <[email protected]> Co-authored-by: Mike Hardy <[email protected]>
Description
This PR stems from the deprecation of the AL (asset library) URL methodology for iOS that started in iOS 8, and now appears to be defunct in iOS 13.
The Storage package relies on this method to fetch a local asset using the AL methodology. This method now returns a
Nilobject that was the probable cause for app crashes in #2762 & #4032.This PR ensures any
Nilobjects resulting from the defunct AL methodology ought to safely return an error message to JS side.Related issues
#2762
#4032
Release Summary
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter