-
Notifications
You must be signed in to change notification settings - Fork 4k
feat!: Firebase iOS SDK version: 10.0.0
#9708
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.
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
Conversation
packages/firebase_storage/firebase_storage/ios/Classes/FLTFirebaseStoragePlugin.m
Outdated
Show resolved
Hide resolved
@@ -27,7 +27,7 @@ Pod::Spec.new do |s| | |||
s.source_files = 'Classes/**/*' | |||
s.public_header_files = 'Classes/**/*.h' | |||
|
|||
s.ios.deployment_target = '9.0' | |||
s.ios.deployment_target = '11.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see on https://github.com/firebase/firebase-ios-sdk/blob/174be0d92bd6706b30a7a9d18f6bbe2319809e03/Firebase.podspec that the ios.deployment_target is still 10.0.
Do we need to manually add the deployment target? Or should CocoaPod just pick it from the dependency? It would me one less thing to maintain from our side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but see what @Salakar thinks as I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can remove it or at a minimum set it back to 10.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed deployment targets for macOS & iOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing deployment targets caused build failures so I guess we do need them. Not sure we should go back to version 10, @Salakar. See documentation. Also - look at the CI logs I linked in build failures. Sample:
of supported deployment target versions is 11.0 to 16.0.99. (in target 'FirebaseAppCheckInterop' from project 'Pods')
/Users/runner/work/flutterfire/flutterfire/tests/ios/Pods/Pods.xcodeproj: warning: The iOS Simulator deployment target 'IPHONEOS_DEPLOYMENT_TARGET' is set to 10.0, but the range of supported deployment target versions is 11.0 to 16.0.99. (in target 'GoogleAppMeasurement' from project 'Pods')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pod relies on 10.0 and above still, but if the docs say they want to support 11+ then sticking to 11 here is fine by me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this makes it a breaking change now across all plugins. I'm also a little concerned about the behaviour of Storage (it's completely changed to a Swift implementation) as I wonder what other edge cases have changed (e.g. exceptions/errors) that we aren't capturing in our CI.
}, | ||
// Fails on emulator since iOS SDK 10. See PR notes: | ||
// https://github.com/firebase/flutterfire/pull/9708 | ||
skip: defaultTargetPlatform == TargetPlatform.iOS || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - read the notes, also about the path
thing. That's a shame! I worked really really hard with Paul and the emulator folks a few months back and we had the storage metadata stuff locked down after a couple rounds of "probe until we understand" + "fix either react-native-firebase or firebase-ios-sdk or firebase-tools". There were a few little corner cases on all sides. May take a bit to tack down the regressions, I can help here as well, as react-native-firebase has a very thorough StorageMetadata test setup after all that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one other thing to note is since storage has moved to swift and they now use fatalError
we can no longer catch things like 'emulator already set' etc, as more things move to swift some of our try/catchers for workarounds are going to be more problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be creating a repro & issue on the firebase-ios-sdk repo with the bugs that I found once we have this and count feature released 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please tag me in that @russellwheatley - I was neck-deep in it so recently I can hopefully assist
10.0.0
10.0.0
🤔
|
@mikehardy yeah, they've just recently started to fail 😓. I'm not sure why. I'm guessing something to do with Firebase Server. Investigating now. |
Whitelisting deeplink domain on Firebase console has solved one of the tests. I think it'll be something along those lines for the other test as well. |
packages/cloud_firestore/cloud_firestore/ios/cloud_firestore.podspec
Outdated
Show resolved
Hide resolved
…odspec Co-authored-by: Mike Hardy <[email protected]>
packages/cloud_firestore/cloud_firestore/macos/cloud_firestore.podspec
Outdated
Show resolved
Hide resolved
// Fails on emulator since iOS SDK 10. See PR notes: | ||
// https://github.com/firebase/flutterfire/pull/9708 | ||
skip: defaultTargetPlatform == TargetPlatform.iOS || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tracking - this will resolve with firebase/firebase-ios-sdk#10370 (or perhaps something in firebase-tools)
Hello, I don't understand, you approved a commit with a storage bug in the emulator ? Skipped getDownloadURL() task as it fails when using the emulator. It fails here. I think because the serialised JSON response has a property that is not of the type [String:String]. Here is the error response: Thanks |
@JavierPerezLavadie This is a very large project, and the storage emulator release schedule is not in this project's control. On balance, sometimes software ships with known issues and for those affected they need to handle with extra care. I believe you can use the storage emulator by not upgrading if the issue affects you. Does it work when you stay on the versions of flutter fire prior to the release? |
I understand but this problem makes the storage on the emulator unusable. I should have waited a bit longer before updating. Yes before the update it worked. I saw here that you already fixed the error? firebase/firebase-ios-sdk#10374 Do you think these PRs for Putfile and getDownload() for the emulator will be in the next update? Thanks |
That is use case dependent, I understand most will probably use the affected method but not all. Not to quibble just to quibble but beware blanket statements in software On the plus side firebase has counts functionality now which is a big deal as announced at firebase summit. You can imagine the desire for some to start incorporating that and you can see the competing pressure for releases
Source control hopefully makes the revert easy after qualification on the new release fails. It is hassle, no doubt, apologies for that.
They are both merged so to the best of my know they will be in firebase-ios-sdk release 10.1.0 yes (or 10.0.1, next release basically) I've never tried integration with the upstream nightly or beta releases but perhaps it is possible to access those by overriding something in firebase core as well https://github.com/firebase/firebase-ios-sdk/tags depending on the urgency for you |
Thank you very much for your detailed answer, I will wait for the next update hoping that it will solve the problem. |
Description
FIRStorageMetadata
bug causing a fatal error. Uploading file fails because it checks for path property.path
property is read only. But you can get around it by using:This allows you to set a
name
property which will be added to the metadata as apath
property here.This needs to be flagged with iOS SDK team.
Updated podspec for plugins. iOS has a minimum deployment target of 11. macOS has a minimum deployment target of 10.13. See documentation.
Skipped
getDownloadURL()
task as it fails when using the emulator. It fails here. I think because the serialised JSON response has a property that is not of the type[String:String]
. Here is the error response:value String "Invalid data returned from the server:{\"name\":\"flutter-tests/writeOnly.txt\",\"bucket\":\"flutterfire-e2e-tests.appspot.com\",\"generation\":\"1665653574035\",\"metageneration\":\"1\",\"contentType\":\"application/octet-stream\",\"timeCreated\":\"2022-10-13T09:32:54.035Z\",\"updated\":\"2022-10-13T09:32:54.035Z\",\"storageClass\":\"STANDARD\",\"size\":\"10\",\"md5Hash\":\"o8152dbQWHKzkt84nfUEKg==\",\"crc32c\":\"4120230975\",\"etag\":\"tHAEijHjZXFls/6B1nPFt/DJ7Ec\",\"downloadTokens\":\"97fec822-84bd-422f-a6ab-82dc30de6029\",\"contentEncoding\":\"identity\",\"contentDisposition\":\"inline\",\"metadata\":{}}"
Note that

metadata
has an empty object as its value. When making the same request on the Firebase server (not the emulator), we get a successful response. It does not contain the propertymetadata
. See:I have skipped this test for now but it is something to flag with iOS SDK team.
initWithDictionary()
when setting metadata (updateMetadata()
) which is a bug.initWithDictionary()
will setinitialMetadata
here. WhenStorageUpdateMetadataTask
callsupdateMetadata.updatedMetadata()
here, it will compareinitialMetadata
and the returned dictionary ofdictionaryRepresentation()
here. It then calls aremove()
function that compares the values and set the values tonil
if "old" metadata property and "new" metadata property are the same. See:I don't understand why it sets the values to
nil
. I also don't understand how it can be "new" & "old" metadata when it's the same metadata used to initializeFIRStorageMetadata
. Either way, we can get around this problem by simply settingcustomMetadata
property the way we have done previously:This should also be flagged with iOS SDK team.
Related Issues
Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?