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.

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

feat!: Firebase iOS SDK version: 10.0.0 #9708

Merged
merged 33 commits into from Oct 18, 2022
Merged

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Oct 11, 2022

Description

  1. nanopb dependency added to Firestore spec to fix zipped Firestore framework which was failing to build due to undefined symbols from that library.
  2. removed duplicate call of emulator as calling it twice causes a fatal error.
  3. fixed 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:
[[FIRStorageMetadata alloc] initWithDictionary:metadata]

This allows you to set a name property which will be added to the metadata as a path property here.

This needs to be flagged with iOS SDK team.

  1. Updated podspec for plugins. iOS has a minimum deployment target of 11. macOS has a minimum deployment target of 10.13. See documentation.

  2. 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 property metadata. See:
Screenshot 2022-10-13 at 10 37 15

I have skipped this test for now but it is something to flag with iOS SDK team.

  1. Cannot initWithDictionary() when setting metadata (updateMetadata() ) which is a bug. initWithDictionary() will set initialMetadata here. When StorageUpdateMetadataTask calls updateMetadata.updatedMetadata() here, it will compare initialMetadata and the returned dictionary of dictionaryRepresentation() here. It then calls a remove() function that compares the values and set the values to nil if "old" metadata property and "new" metadata property are the same. See:

Screenshot 2022-10-13 at 11 34 11

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 initialize FIRStorageMetadata. Either way, we can get around this problem by simply setting customMetadata property the way we have done previously:

FIRStorageMetadata *metadata = [[FIRStorageMetadata alloc] init];
metadata.customMetadata = dictionary[kFLTFirebaseStorageKeyCustomMetadata];

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@@ -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'
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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')

Copy link
Member

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!

Copy link
Member Author

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.

@russellwheatley russellwheatley marked this pull request as ready for review October 14, 2022 11:57
},
// Fails on emulator since iOS SDK 10. See PR notes:
// https://github.com/firebase/flutterfire/pull/9708
skip: defaultTargetPlatform == TargetPlatform.iOS ||
Copy link
Contributor

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

Copy link
Member

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

Copy link
Member Author

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 😄

Copy link
Contributor

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

@russellwheatley russellwheatley changed the title feat: Firebase iOS SDK version: 10.0.0 feat!: Firebase iOS SDK version: 10.0.0 Oct 17, 2022
@mikehardy
Copy link
Contributor

🤔


  firebase_dynamic_links buildShortLink
     ✘ build short dynamic links
       [E]: type 'int' is not a subtype of type 'String' in type cast

  firebase_dynamic_links getInitialLink
     ✔ initial link

  firebase_dynamic_links getDynamicLink
     ✘ dynamic link using uri created on Firebase console
       [E]: type '_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'String' in type cast
     ✔ Universal link error for URL that cannot be parsed

https://github.com/firebase/flutterfire/actions/runs/3265837960/jobs/5369204292#step:12:1040

@russellwheatley
Copy link
Member Author

🤔


  firebase_dynamic_links buildShortLink
     ✘ build short dynamic links
       [E]: type 'int' is not a subtype of type 'String' in type cast

  firebase_dynamic_links getInitialLink
     ✔ initial link

  firebase_dynamic_links getDynamicLink
     ✘ dynamic link using uri created on Firebase console
       [E]: type '_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'String' in type cast
     ✔ Universal link error for URL that cannot be parsed

firebase/flutterfire/actions/runs/3265837960/jobs/5369204292#step:12:1040

@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.

@russellwheatley
Copy link
Member Author

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.

…odspec

Co-authored-by: Mike Hardy <github@mikehardy.net>
Comment on lines +141 to +143
// Fails on emulator since iOS SDK 10. See PR notes:
// https://github.com/firebase/flutterfire/pull/9708
skip: defaultTargetPlatform == TargetPlatform.iOS ||
Copy link
Contributor

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)

@JavierPerezLavadie
Copy link

JavierPerezLavadie commented Oct 25, 2022

Hello, I don't understand, you approved a commit with a storage bug in the emulator ?
Now i can't use the emulator for my app anymore, do you have an alternative to getDownloadURL() on 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

@mikehardy
Copy link
Contributor

@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?

@JavierPerezLavadie
Copy link

@mikehardy

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

@mikehardy
Copy link
Contributor

@mikehardy

I understand but this problem makes the storage on the emulator unusable.

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

I should have waited a bit longer before updating.

Source control hopefully makes the revert easy after qualification on the new release fails. It is hassle, no doubt, apologies for that.

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?

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

@JavierPerezLavadie
Copy link

Thank you very much for your detailed answer, I will wait for the next update hoping that it will solve the problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants