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

Remove Protobuf from the ABT SDK #5890

Merged
merged 42 commits into from Jul 6, 2020
Merged

Remove Protobuf from the ABT SDK #5890

merged 42 commits into from Jul 6, 2020

Conversation

christibbs
Copy link
Contributor

@christibbs christibbs commented Jun 24, 2020

  • 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

Copy link
Member

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

FirebaseABTesting.podspec Show resolved Hide resolved
…p RemoteConfigExperiment to use new ABTExperimentPayload object
…involves extending the ABTExperimentPayload initializer to accept different values for overflowPolicy and experimentStartTime
@google-oss-bot google-oss-bot added the api: inappmessaging Firebase In App Messaging label Jun 24, 2020
@paulb777
Copy link
Member

The travis issues reproduce locally with Xcode 10.3 targeting an iPhone5 simulator. Likely there is a 32bit/64bit data processing issue.

Screen Shot 2020-06-26 at 9 45 18 AM

@@ -17,15 +17,15 @@
@class ABTExperimentPayload;

// Forward declaration to avoid importing into the module header
typedef NS_ENUM(int32_t, ABTExperimentPayload_ExperimentOverflowPolicy);
Copy link
Member

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

Copy link
Contributor Author

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?

@christibbs
Copy link
Contributor Author

The travis issues reproduce locally with Xcode 10.3 targeting an iPhone5 simulator. Likely there is a 32bit/64bit data processing issue.

Screen Shot 2020-06-26 at 9 45 18 AM

Thanks for looking into that. I didn't have iPhone 5 on my simulators list. I changed the millisecond properties to long long in de42dd0.

@paulb777
Copy link
Member

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?

@paulb777 paulb777 mentioned this pull request Jun 26, 2020
@ryanwilson
Copy link
Member

After considering it, I think I'm fine with bumping the major version of ABTesting without a Firebase major version.

@christibbs
Copy link
Contributor Author

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.

Copy link
Member

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

One more change and then LGTM:

Bump the FirebaseABTesting version in Firebase.podspec

@ryanwilson ryanwilson added this to In progress in Swift Package Manager Support via automation Jul 3, 2020
@christibbs
Copy link
Contributor Author

One more change and then LGTM:

Bump the FirebaseABTesting version in Firebase.podspec

Done in 8e68e3e.

Copy link
Member

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

@christibbs christibbs merged commit 9b48451 into master Jul 6, 2020
Swift Package Manager Support automation moved this from In progress to Done Jul 6, 2020
granluo pushed a commit that referenced this pull request Jul 7, 2020
* 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>
@firebase firebase locked and limited conversation to collaborators Aug 6, 2020
@paulb777 paulb777 deleted the abt-remove-protobuf branch August 19, 2020 00:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants