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
Remove Protobuf from the ABT SDK #5890
Conversation
…figure out how RC serializes the NSData objects passed to FIRExperimentController in updateExperimentsWithServiceOrigin:
…r classes that depend on it
…ertyTest, add tests for ABTExperimentPayload
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.
Thanks Chris. One nit so far and it looks like some work to get CI green.
If this PR will break RC on master, it should be updated here along with its podspec dependency version on ABT to the next minor version.
I'll do a more thorough review tomorrow.
…p RemoteConfigExperiment to use new ABTExperimentPayload object
…involves extending the ABTExperimentPayload initializer to accept different values for overflowPolicy and experimentStartTime
…s-sdk into abt-remove-protobuf
…s-sdk into abt-remove-protobuf
@@ -17,15 +17,15 @@ | |||
@class ABTExperimentPayload; | |||
|
|||
// Forward declaration to avoid importing into the module header | |||
typedef NS_ENUM(int32_t, ABTExperimentPayload_ExperimentOverflowPolicy); |
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.
Changing the type name will break RemoteConfig users who lock their version and do a pod update
. Can we keep the type name unchanged until the next major version.
Otherwise, we could consider bumping the major version since we don't really have a public API for ABT.
cc: @ryanwilson
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'd prefer bumping the major version since the underscores aren't idiomatic Objective-C. What do you guys think?
Thanks for looking into that. I didn't have iPhone 5 on my simulators list. I changed the millisecond properties to |
…s-sdk into abt-remove-protobuf
I'm not sure about changing the double floating types to int types. I would have thought it would be changing the intValue's to int64Value's - https://developer.apple.com/documentation/foundation/nsnumber/1416870-int64value. @ryanwilson Will you answer the API question? Are we ok to bump the major version of FirebaseABTesting without a Firebase major version since there are no public ABTesting APIs? |
After considering it, I think I'm fine with bumping the major version of ABTesting without a Firebase major version. |
Great. Bumped major version in 84d865f. |
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 more change and then LGTM:
Bump the FirebaseABTesting version in Firebase.podspec
Done in 8e68e3e. |
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.
Thanks!
* Remove Protobuf dependency, add struct-ish class that mimics ABTExperimentPayload * Remove a lot of the compiler errors, but not all of them. We need to figure out how RC serializes the NSData objects passed to FIRExperimentController in updateExperimentsWithServiceOrigin: * Add ABTExperimentPayload class that replaces generated proto, refactor classes that depend on it * Add unit testing class for ABTExperimentPayload * Updates to ABTExperimentPayload API, tests for ABTConditionalUserPropertyTest, add tests for ABTExperimentPayload * Update podspec to pull in testing resources * Add utility class for parsing test files. Use it in ABTExperimentPayloadTests * Fix FIRExperimentController tests * Better documentation * Run scripts/styles.sh * Fix whitespace * Remove protobuf imports * Bump pod spec version for ABT SDK, depend on new ABT SDK in RC. Fix up RemoteConfigExperiment to use new ABTExperimentPayload object * scripts/style.sh * Update In-app messaging to use new ABTExperimentPayload class, which involves extending the ABTExperimentPayload initializer to accept different values for overflowPolicy and experimentStartTime * Delete test payload * Revert scripts/check_imports.swift * Better initialization tags for ABTExperimentLite * Update CHANGELOGs * Remove private headers in podspec, use integerValue to parse integers * Refactor ABTExperimentPayload header into Private headers. Fix integer types for millisecond metadata * scripts/styles.sh * Fix integer parsing * Fix copyrights * Fix CHANGELOG typo: * Add private_header_files to public_header_files * Try just using NSInteger for experimentStartTimeMillis * Bump FIAM dependency on ABTesting * Use long long for milli parameters (32-bit apps) * Use longlong in test timestamps * Don't cast to nsnumber in tests * Use in64_t * Fix Test Overflow * Bump major version on ABT SDK * Bump ABTesting version in Firebase.podspec Co-authored-by: Paul Beusterien <paulbeusterien@google.com>
Removes the generated
ABTExperimentPayload
from the ABT SDK.Replaces it with an NSObject subclass that contains the same metadata.
Fixes up unit tests.
Updates Remote Config and In-App Messaging to consume "new" API