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

[Config] Merge RemoteConfig and its Swift extension in a non-breaking way #11720

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Aug 23, 2023

The test specs to test the Swift library code are still part of the RemoteConfigSwift.podspec. I'm leaving them there temporarily to reduce the amount of changes to get this tested across CI.

I have also left the Swift library sources in the RemoteConfigSwift directory. These and the tests mentioned earlier should likely be moved into a reorganized RemoteConfig directory.

Different to #11386 in that this PR is aimed to merge the two libraries in a non-breaking way.


CocoaPods approach

  • The FirebaseRemoteConfigSwift pod now exports the FirebaseRemoteConfig module and has no APIs.
  • The Swift extension sources are added to the FirebaseRemoteConfig pod, making it a mixed lang. pod.

SPM approach

  • The FirebaseRemoteConfigSwift target now exports the FirebaseRemoteConfig module and has no APIs.
  • FirebaseRemoteConfigSwift's previous Swift contents are moved into FirebaseRemoteConfig, and the ObjC sources that were in FirebaseRemoteConfig are moved into an internal target named FirebaseRemoteConfigInternal. See the SPM-specific workaround for re-exporting the ObjC API from the FirebaseRemoteConfig target that now contains only Swift extension APIs.

Test cases for ensuring this is non-breaking

  • Test ObjC app importing FirebaseRemoteConfig via headers still builds/runs
    • CocoaPods tested via config / sample-build-test
    • SPM via spm / client-app (see SwiftPMTests/ClientApp/ClientApp/objc-header-import-test.m)
    • Zip via Legacy RC quickstart [Manual]
  • Test ObjC app importing FirebaseRemoteConfig via module import still builds/runs
    • CocoaPods via CocoaPods via Legacy RC quickstart [Manual]
    • SPM via spm / client-app (see SwiftPMTests/ClientApp/ClientApp/objc-module-import-test.m)
    • Zip via Legacy RC quickstart [Manual]
  • See if an ObjC app can currently import FirebaseRemoteConfigSwift and/or FirebaseRemoteConfigSwift-Swift.h
    • I'd be surprised if there are ObjC apps importing FirebaseRemoteConfigSwift, but I'd like to understand the implications if one does.
      • I diff'd the FirebaseRemoteConfigSwift-Swift.h and there was no functional difference. Should be good here.
  • Test Swift app importing FirebaseRemoteConfigSwift via module import still builds/runs
    • CocoaPods via remoteconfig / quickstart
    • SPM via spm / client-app AND SwiftUI RC app [Manual]
    • Zip via zip workflow (see comment below)
  • Test Swift app importing FirebaseRemoteConfig via module import still builds/runs
    • CocoaPods via Legacy RC quickstart [Manual]
    • SPM via Legacy RC quickstart [Manual]
    • Zip via Legacy RC quickstart [Manual]

Follow-up work:

  • CHANGELOG.md ([Config] Refactor Swift sources out of FirebaseRemoteConfigSwift #11747)
  • Add CocoaPods scheme to ClientApp to test podspec changes ([Infra] Add CocoaPods test app to test library imports #11734)
  • Move Swift sources into FirebaseRemoteConfig/ ([Config] Refactor Swift sources out of FirebaseRemoteConfigSwift #11747)
  • Consider implications of doing/not doing SPM hack for CocoaPods
    • Up until this PR, there was no FirebaseRemoteConfig-Swift.h generated by SPM or CocoaPods because FirebaseRemoteConfig had no Swift code. With this PR, it seems that in SPM, a Clang import of @import FirebaseRemoteConfig; requires the generated header but the same import in CocoaPods does not. The reason is because of the module maps.
    // SPM module map (remember, `FirebaseRemoteConfig` is Swift only!)
    module FirebaseRemoteConfig {
      header "FirebaseRemoteConfig-Swift.h"
      export *
    }
    
    // cocoapods module map (remember, `FirebaseRemoteConfig` is mixed lang!)
    framework module FirebaseRemoteConfig {
      umbrella header "FirebaseRemoteConfig-umbrella.h"
      export *
      module * { export * }
    }
    
    module FirebaseRemoteConfig.Swift {
      header "FirebaseRemoteConfig-Swift.h"
      requires objc
    }
    
    • So we don't need to do the hack in CocoaPods because the module import will expand directly to the headers (rather than going through a -Swift.h header in SPM).
  • Add API availability guards for other platforms
  • Create API Review.
    • API for SPM workaround is not needed as it will be unavailable on all platforms.
  • SPM-only: Consider whether RC import needs to be exported...
    • It does need to be @_exported because without it, SPM clients wouldn't get the ObjC API from the SPM FirebaseRemoteConfigInternal target.
  • Figure out a solution for the tests specs in FirebaseRemoteConfigSwift.podspec
  • Add Zip scheme to ClientApp to test zip changes
    • This is currently too difficult to do quickly.

@google-oss-bot
Copy link

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

Generated by 🚫 Danger

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2023

Apple API Diff Report

Commit: 8d8012e
Last updated: Tue Aug 29 11:40 PDT 2023
View workflow logs & download artifacts


[REMOVED] FirebaseRemoteConfigSwift

[REMOVED] FirebaseRemoteConfigSwift
Swift:
-      public enum RemoteConfigCodableError : Error
-        case invalidSetDefaultsInput ( String )
-      public enum RemoteConfigValueCodableError : Error
-        case unsupportedType ( String )
-      public extension RemoteConfig
-        func decoded < Value > ( asType : Value . Type = Value . self ) throws -> Value where Value : Decodable
-        func setDefaults < Value > ( from value : Value ) throws where Value : Encodable
-        subscript < T > ( decodedValue key : String ) -> T ? where T : Decodable { get }
-        subscript ( jsonValue key : String ) -> [ String : AnyHashable ]? { get }
-      public extension RemoteConfigValue
-        func decoded < Value > ( asType : Value . Type = Value . self ) throws -> Value where Value : Decodable
-      @available(iOS 14.0, macOS 11.0, tvOS 14.0, watchOS 7.0, *) @propertyWrapper public struct RemoteConfigProperty < T > : DynamicProperty where T : Decodable
-        public let key : String
-        public var wrappedValue : T { get }
-        public init ( key : String , fallback : T )

@google-oss-bot
Copy link

Coverage Report 1

Affected Products

  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

    Overall coverage changed from 73.88% (327457a) to 72.24% (8d8012e) by -1.65%.

    FilenameBase (327457a)Merge (8d8012e)Diff
    Codable.swift?0.00%?
    FirebaseRemoteConfigValueDecoderHelper.swift?0.00%?
    RemoteConfigProperty.swift?0.00%?
    RemoteConfigValueObservable.swift?0.00%?
    Value.swift?0.00%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/jHElGdEhsH.html

@ncooke3 ncooke3 requested a review from paulb777 August 23, 2023 21:35
@ncooke3
Copy link
Member Author

ncooke3 commented Aug 24, 2023

zip run: https://github.com/firebase/firebase-ios-sdk/actions/runs/5958843237

edit: zip run is green 🎉

@ncooke3 ncooke3 requested a review from paulb777 August 24, 2023 21:01
@ncooke3
Copy link
Member Author

ncooke3 commented Aug 24, 2023

@paulb777, I've completed manual testing.

As far as outstanding work, I'm thinking:

  • Swift sources from FirebaseRemoteConfigSwift/ should be moved into FirebaseRemoteConfig/
  • The FirebaseRemoteConfigSwift.podspec has test specs to test the Swift API. We probably should move these test specs to the FirebaseRemoteConfig.podspec. One caveat is that we can't test these test specs with pod lib lint currently because they require some setup. So, I'll need to explore that more. WDYT?

Do you think the above work items should be done before merging into master? If so, I was going to propose merging this in a feature branch after a LGTM.

edit: I realize I could also just do this follow-up work into master too...

@ncooke3 ncooke3 marked this pull request as ready for review August 24, 2023 21:07
@paulb777
Copy link
Member

I don't feel strongly about not merging into master as long as we consider the state releasable and expect to do the rest before the next code freeze.

Note that pod lib lint has a --test-specs option to specify a subset of test-specs to run. That should enable us to continue to run the other tests the same way they're running now from the remoteconfig job.

@ncooke3 ncooke3 enabled auto-merge (squash) August 24, 2023 21:15
@ncooke3 ncooke3 merged commit 05b088d into master Aug 24, 2023
77 checks passed
@ncooke3 ncooke3 deleted the nc/rc-pod-merge-non-breaking branch August 24, 2023 21:15
@sunmou99 sunmou99 restored the nc/rc-pod-merge-non-breaking branch August 29, 2023 19:52
ncooke3 added a commit that referenced this pull request Sep 1, 2023
@firebase firebase locked and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants