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

RC Swift pod and codable #9084

Merged
merged 29 commits into from
Jan 18, 2022
Merged

RC Swift pod and codable #9084

merged 29 commits into from
Jan 18, 2022

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Dec 12, 2021

Fix #6883 Remote Config Codable support

  • Decode Remote Config Value
  • Decode Config into a specified struct
  • Set defaults via an encoded struct
  • Added tests

This PR also does the following:

  • Adds a subscript overload API (and tests) to get typed values from the top-level config API
  • Initial Remote Config Swift library for CocoaPods and SwiftPM
  • Migrate Swift tests to Swift directory
  • Associate CI updates to build and test the new library in both CocoaPods and SwiftPM
  • Convert Fake Console tests to use MockConfig from SharedTestUtils instead of a dummy GoogleService-Info.plist file
  • Made Tests README.md more visible and include both types of Swift integration tests
  • Consolidated test setup and Constants to APITestBase.swift and Constants.swift

Demonstration Quick Start PR at firebase/quickstart-ios#1332

Investigated Questions:

  • Is there a better way to decode RemoteConfigValue's than modifying FirebaseDataEncoder.swift?
    • Not that we can find. RemoteConfigValue is an Objective C type. We don't want to decode to it, we want to decode to it to primitive types
  • Are the subscript overload API's useful or are they too redundant with the decoder API's?
    • They're a useful abstraction and share the implementation with the RemoteConfigValue decoded implementation

Open Question:

  • The decoder supports Date types, but it's not yet clear how that should work with RemoteConfig or other Firebase products.

Next Steps:

  • API review(s) in progress - internally visible at go/firebase-rc-swift-type-safety

Thanks and congratulations to @fumito-ito for being a first mover in the RC Codable space with https://github.com/fumito-ito/SwiftyRemoteConfig! Any key additional features from that we should consider for adding here?

@paulb777 paulb777 marked this pull request as draft December 12, 2021 18:33
@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

Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting stuff– thanks! I added some questions, comments, and suggestions.

FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
@paulb777 paulb777 changed the base branch from codable-refactor3 to codable-refactor-save December 14, 2021 23:56
@paulb777 paulb777 force-pushed the pb-rc-codable branch 2 times, most recently from 5de4087 to 7a08f17 Compare December 15, 2021 01:44
@paulb777 paulb777 changed the base branch from codable-refactor-save to codable-refactor3 December 15, 2021 01:45
Copy link
Member Author

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the deep review @ncooke3 💯

FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/Tests/SwiftAPI/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
Base automatically changed from codable-refactor3 to master December 16, 2021 20:13
@google-oss-bot
Copy link

google-oss-bot commented Dec 21, 2021

Size Report 1

Affected Products

  • FirebaseDatabase

    TypeBase (c7456c9)Merge (41741d0)Diff
    CocoaPods?1.24 MB? (?)
  • FirebaseRemoteConfig

    TypeBase (c7456c9)Merge (41741d0)Diff
    CocoaPods?571 kB? (?)

Test Logs

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

@google-oss-bot
Copy link

google-oss-bot commented Dec 21, 2021

Coverage Report 1

Affected Products

  • FirebaseDatabase-iOS-FirebaseDatabase.framework

    Overall coverage changed from ? (c7456c9) to 56.74% (41741d0) by ?.

    102 individual files with coverage change

    FilenameBase (c7456c9)Merge (41741d0)Diff
    APLevelDB.mm?61.30%?
    FAckUserWrite.m?83.33%?
    FArraySortedDictionary.m?97.49%?
    FAtomicNumber.m?100.00%?
    fbase64.c?32.00%?
    FCacheNode.m?100.00%?
    FCachePolicy.m?17.14%?
    FCancelEvent.m?0.00%?
    FChange.m?88.24%?
    FChildChangeAccumulator.m?62.32%?
    FChildEventRegistration.m?0.00%?
    FChildrenNode.m?90.40%?
    FClock.m?100.00%?
    FCompoundHash.m?97.70%?
    FCompoundWrite.m?91.12%?
    FConnection.m?26.09%?
    FDataEvent.m?29.41%?
    FEmptyNode.m?100.00%?
    FEventEmitter.m?0.00%?
    FEventGenerator.m?95.37%?
    FEventRaiser.m?30.00%?
    FImmutableSortedDictionary.m?31.03%?
    FImmutableSortedSet.m?53.01%?
    FImmutableTree.m?85.02%?
    FIndex.m?91.67%?
    FIndexedFilter.m?95.65%?
    FIndexedNode.m?97.06%?
    FIRDatabase.m?14.93%?
    FIRDatabaseComponent.m?19.23%?
    FIRDatabaseConfig.m?59.62%?
    FIRDatabaseConnectionContextProvider.m?96.67%?
    FIRDatabaseQuery.m?3.86%?
    FIRDatabaseReference.m?6.41%?
    FIRDataSnapshot.m?89.71%?
    FIRMutableData.m?75.51%?
    FIRRetryHelper.m?82.14%?
    FIRServerValue.m?0.00%?
    FIRTransactionResult.m?0.00%?
    FKeepSyncedEventRegistration.m?0.00%?
    FKeyIndex.m?60.56%?
    FLeafNode.m?86.98%?
    FLevelDBStorageEngine.m?72.66%?
    FLimitedFilter.m?96.79%?
    FListenComplete.m?0.00%?
    FLLRBEmptyNode.m?75.00%?
    FLLRBValueNode.m?84.83%?
    FMaxNode.m?60.61%?
    FMerge.m?87.18%?
    FNamedNode.m?62.32%?
    FNextPushId.m?62.00%?
    FOperationSource.m?89.29%?
    FOverwrite.m?81.48%?
    FPath.m?93.86%?
    FPathIndex.m?88.75%?
    FPendingPut.m?0.00%?
    FPersistenceManager.m?54.55%?
    FPersistentConnection.m?20.95%?
    FPriorityIndex.m?87.84%?
    FPruneForest.m?93.08%?
    FQueryParams.m?95.86%?
    FQuerySpec.m?86.36%?
    FRangedFilter.m?93.67%?
    FRangeMerge.m?94.06%?
    FRepo.m?13.42%?
    FRepoInfo.m?73.00%?
    FRepoManager.m?29.17%?
    FServerValues.m?50.85%?
    FSnapshotHolder.m?100.00%?
    FSnapshotUtilities.m?60.45%?
    FSparseSnapshotTree.m?97.20%?
    FSRWebSocket.m?38.73%?
    FStringUtilities.m?79.41%?
    FSyncPoint.m?86.99%?
    FSyncTree.m?70.73%?
    FTrackedQuery.m?75.38%?
    FTrackedQueryManager.m?90.00%?
    FTransformedEnumerator.m?100.00%?
    FTree.m?6.04%?
    FTreeNode.m?100.00%?
    FTreeSortedDictionary.m?95.79%?
    FTreeSortedDictionaryEnumerator.m?100.00%?
    FTupleNodePath.m?0.00%?
    FTupleObjectNode.m?0.00%?
    FTuplePathValue.m?100.00%?
    FTupleRemovedQueriesEvents.m?100.00%?
    FTupleSetIdPath.m?0.00%?
    FTupleStringNode.m?0.00%?
    FTupleTransaction.m?0.00%?
    FTupleUserCallback.m?0.00%?
    FUtilities.m?74.01%?
    FValidation.m?25.58%?
    FValueEventRegistration.m?0.00%?
    FValueIndex.m?48.53%?
    FView.m?78.98%?
    FViewCache.m?100.00%?
    FViewProcessor.m?90.86%?
    FViewProcessorResult.m?100.00%?
    FWebSocketConnection.m?41.91%?
    FWriteRecord.m?45.65%?
    FWriteTree.m?73.67%?
    FWriteTreeRef.m?100.00%?
    NSData+SRB64Additions.m?81.82%?

  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

    Overall coverage changed from ? (c7456c9) to 78.97% (41741d0) by ?.

    11 individual files with coverage change

    FilenameBase (c7456c9)Merge (41741d0)Diff
    FIRConfigValue.m?58.70%?
    FIRRemoteConfig.m?83.44%?
    FIRRemoteConfigComponent.m?97.06%?
    RCNConfigContent.m?80.68%?
    RCNConfigDBManager.m?75.95%?
    RCNConfigExperiment.m?92.25%?
    RCNConfigFetch.m?79.40%?
    RCNConfigSettings.m?62.10%?
    RCNDevice.m?81.29%?
    RCNPersonalization.m?89.74%?
    RCNUserDefaultsManager.m?97.89%?

Test Logs

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

@paulb777 paulb777 changed the title Infra for RC Swift pod and codable RC Swift pod and codable Dec 28, 2021
@paulb777 paulb777 force-pushed the pb-rc-codable branch 2 times, most recently from ceb0be9 to a058a30 Compare January 4, 2022 22:29
.github/workflows/remoteconfig.yml Show resolved Hide resolved
.github/workflows/remoteconfig.yml Show resolved Hide resolved
.github/workflows/remoteconfig.yml Show resolved Hide resolved
FirebaseRemoteConfig.podspec Show resolved Hide resolved
scripts/build.sh Show resolved Hide resolved
scripts/build.sh Show resolved Hide resolved
@paulb777 paulb777 marked this pull request as ready for review January 4, 2022 23:50
@peterfriese
Copy link
Contributor

Open Questions:

  • Is there a better way to decode RemoteConfigValue's than modifying FirebaseDataEncoder.swift?
  • Are the subscript overload API's useful or are they too redundant with the decoder API's?

Next Steps:

Great stuff! Two questions / suggestions:

  1. I think using "RemoteConfig" instead of "RC" as the prefix for the public facing types might be nicer, but that's my personal preference.
  2. Any thoughts on adding a property wrapper for accessing remote config?

Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the test files yet, but here's the first pass before doing so.

FirebaseRemoteConfig.podspec Show resolved Hide resolved
FirebaseRemoteConfigSwift.podspec Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift.podspec Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Value.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Sources/Codable.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfigSwift/Tests/SwiftAPI/APITestBase.swift Outdated Show resolved Hide resolved
@paulb777 paulb777 mentioned this pull request Jan 6, 2022
7 tasks
@paulb777
Copy link
Member Author

The API changes were approved today and I plan to merge on Tuesday.

@paulb777 paulb777 added this to the 8.12.0 - M111 milestone Jan 14, 2022
@paulb777 paulb777 merged commit 2246d8f into master Jan 18, 2022
@paulb777 paulb777 deleted the pb-rc-codable branch January 18, 2022 15:25
leotianlizhan pushed a commit that referenced this pull request Feb 1, 2022
@firebase firebase locked and limited conversation to collaborators Feb 18, 2022
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.

FR: Add RemoteConfig Decoder and Encoder
7 participants