Functions Codable after sharing implementation with RTDB

[paulb777]

Umbrella PR that rolls up the following

#8853 from @mortenbekditlevsen to share Codable implementation
#8854 from @mortenbekditlevsen to add Firebase Functions Codable implementation
Additional Functions Codable APIs cherry-picked from @peterfriese's #9057
CI implementation cherry-picked from @paulb777's #8983

Reviewers, you may want to review commit by commit

The FirebaseFunctionsSwift CocoaPods implementation, additional CI and associated documentation will come in follow up PR(s)

I plan to merge this PR without squashing to keep the contribution history in master.

[google-oss-bot]

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

[google-oss-bot]

Coverage Report

Affected SDKs

• FirebaseDatabase-iOS-FirebaseDatabase.framework

  SDK overall coverage changed from 56.69% (944416b) to 56.79% (5d9ff29) by +0.09%.

  Filename	Base (944416b)	Head (5d9ff29)	Diff
  FSRWebSocket.m	38.73%	40.82%	+2.10%
  FWebSocketConnection.m	41.91%	37.34%	-4.56%

Test Logs

• Base (944416b3): https://github.com/firebase/firebase-ios-sdk/actions/runs/1588492663
• Head (5d9ff298): https://github.com/firebase/firebase-ios-sdk/actions/runs/1589076058

[paulb777]

@mortenbekditlevsen @peterfriese Please confirm I merged everything correctly.

@ryanwilson This should now be a one-stop PR for review of the Codable changes for Functions and RTDB

cc: @schmidt-sebastian

[ryanwilson]

Thanks a ton for putting this together, @paulb777!

[ryanwilson]

I struggle with the naming of StructuredEncoder - it's very generic which accurately matches the implementation, but I worry that it could be confused with a non-Firebase type and used for other purposes.

What about something like FirebaseDataEncoder/Decoder? I recognize it's code in third_party but it's a fork that we want to use exclusively for Firebase Codable implementations and that helps set the expectations.

It would also be useful to update to the latest implementation, along with a comment of the hash we retrieved it at for later comparisons.

[paulb777]

Updated Callable API to use requestAs and responseAs instead of requestType and requestAs

Enabled second way of using API after an internal conversation. See the new test

    func testScalarAsyncAlternateSignature() async throws {
      let function: Callable<Int16, Int> = functions.httpsCallable("scalarTest")
      let result = try await function.call(17)
      XCTAssertEqual(result, 76)
    }

versus

func testScalarAsync() async throws {
      let function = functions.httpsCallable(
        "scalarTest",
        requestAs: Int16.self,
        responseAs: Int.self
      )

      let result = try await function.call(17)
      XCTAssertEqual(result, 76)
    }

[mortenbekditlevsen]

I am glad to see that this got through your internal API review process.

Regarding the choice to supply both explicit typing of the callable and inferred typing:
Should this design choice form precedence for other similar API? As in: should DocumentSnapshot then also have an inferred version of

  public func data<T: Decodable>(as type: T.Type,
                                 with serverTimestampBehavior: ServerTimestampBehavior = .none,
                                 decoder: Firestore.Decoder? = nil) throws -> T?

without the as type: T.Type? And the same in other similar situations?

And if not: Why might that be?

[paulb777]

@mortenbekditlevsen Thanks for the review. Comments addressed in bdacb29

[paulb777]

@mortenbekditlevsen The design choice of supplying both explicit typing of the callable and inferred typing sounds like a good standard to me. @ryanwilson What do you think?

BTW, we're not completely through the internal API review yet - but hopefully getting close. 😄

[ryanwilson]

I still need to look at the IntegrationTests.swift but looking good so far!

[ryanwilson]

Regarding the choice to supply both explicit typing of the callable and inferred typing: Should this design choice form precedence for other similar API? As in: should DocumentSnapshot then also have an inferred version of

  public func data<T: Decodable>(as type: T.Type,
                                 with serverTimestampBehavior: ServerTimestampBehavior = .none,
                                 decoder: Firestore.Decoder? = nil) throws -> T?

without the as type: T.Type? And the same in other similar situations?

And if not: Why might that be?

@mortenbekditlevsen The design choice of supplying both explicit typing of the callable and inferred typing sounds like a good standard to me. @ryanwilson What do you think?

BTW, we're not completely through the internal API review yet - but hopefully getting close. 😄

That's a really good thought and question. I hadn't considered it since we're in a way duplicating our API surface here, but in this case I think it makes sense since the latter call (explicitly declaring the type and not providing extra arguments) is a much cleaner API due to the ordering (not the first arg) + passing 2 types instead of 1. We don't want to have any APIs that require declaring the type, but in this case it's a pretty huge improvement to readability.

For the other APIs it's worth considering, although I'd want to test out code completion and what sort of help Xcode gives when you get it wrong (don't declare the type and don't provide arguments. One other downside of the explicit declared types is when you separate the type declaration from the call site, it makes it less obvious what's going on. Ex:

private let myCallable: Callable<Request, Result>

// Some other code...

init() {
  myCallable = Functions.functions().httpsCallable("myFunction")
}

Don't know if I want to commit to one way or the other right this second but thanks for pointing it out and we'll have to evaluate it!

[peterfriese]

Found a couple of typos. Also, should we add code samples for calling using inferred types?

[paulb777]

Thanks @peterfriese !

[google-oss-bot]

Binary Size Report

Affected SDKs

• FirebaseDatabase

  Type	Base (ea6bf3a)	Head (a7076d3)	Diff
  CocoaPods	?	1.24 MB	? (?)

• FirebaseFunctions

  Type	Base (ea6bf3a)	Head (a7076d3)	Diff
  CocoaPods	?	504 kB	? (?)

Test Logs

• Head (a7076d3a): https://github.com/firebase/firebase-ios-sdk/actions/runs/1584883319

[paulb777]

As discussed above, I renamed StructuredEncoder to FirebaseDataEncoder.

All comments should be addressed and looking for approvals from @ryanwilson and @peterfriese and optionally @ncooke3, @schmidt-sebastian, and anyone else interested.

[paulb777]

I squashed the commits from 15 to 5.

[ryanwilson]

Just a couple of nits then the tests LGTM, thanks!

[paulb777]

Thanks @ryanwilson !

[peterfriese]

LGTM - excited to see this land!