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

Add core support with interop for Segmentation SDK. #3430

Merged
merged 10 commits into from Jul 25, 2019
Merged

Conversation

@dmandar
Copy link
Contributor

@dmandar dmandar commented Jul 24, 2019

Add core support with interop for Segmentation SDK. Also update header file location to within sources folder.

@@ -19,3 +19,4 @@
NSString *const kFirebaseCoreErrorDomain = @"com.firebase.core";
NSString *const kFirebasePerfErrorDomain = @"com.firebase.perf";
NSString *const kFirebaseStorageErrorDomain = @"com.firebase.storage";
NSString *const kFirebaseSegmentationErrorDomain = @"com.firebase.segmentation";
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Delete. FIRErrors is deprecated and nothing new should be added.

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Done.

@@ -22,3 +22,4 @@ extern NSString *const kFirebaseErrorDomain;
extern NSString *const kFirebaseConfigErrorDomain;
extern NSString *const kFirebaseCoreErrorDomain;
extern NSString *const kFirebasePerfErrorDomain;
extern NSString *const kFirebaseSegmentationErrorDomain;
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Delete. Same as above.

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Done.

#import "FIRSegmentation.h"
#import "FirebaseSegmentation/Sources/Public/FIRSegmentation.h"

#import "FIRAppInternal.h"
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Should be #import <FirebaseCore/FIRAppInternal.h> for this and other Core Private headers.

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Done.

@@ -0,0 +1,86 @@
#import "FIRSegmentationComponent.h"

#import "FIRAppInternal.h"
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Use module import format here too.

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Done.


@end

/// A concrete implementation for FIRSegmentationInterop to create Segmentation instances and
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Is there a need for FIRSegmentationInterop?

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Is there a reason not to? What is the alternative? I thought all products were moving to using interop.

Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Interop is only needed for products that want to manage their interface to other Firebase dependencies separately from their library implementation. Will Segmentation have Firebase pod dependencies?

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Yes, Segmentation will need to depend on IID and FIS.

@@ -21,7 +21,7 @@ Firebase Segmentation enables you to associate your custom application instance
s.prefix_header_file = false

s.source_files = 'FirebaseSegmentation/Sources/**/*'
s.public_header_files = 'FirebaseSegmentation/Public/*.h'
s.public_header_files = 'FirebaseSegmentation/Sources/Public/*.h'

s.dependency 'FirebaseCore', '~> 6.0'
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

6.1

Copy link
Contributor Author

@dmandar dmandar Jul 25, 2019

Done.

@dmandar dmandar requested a review from paulb777 Jul 25, 2019
FirebaseSegmentation/Sources/FIRSegmentationComponent.m Outdated Show resolved Hide resolved

@end

/// A concrete implementation for FIRSegmentationInterop to create Segmentation instances and
Copy link
Member

@paulb777 paulb777 Jul 25, 2019

Interop is only needed for products that want to manage their interface to other Firebase dependencies separately from their library implementation. Will Segmentation have Firebase pod dependencies?

@paulb777
Copy link
Member

@paulb777 paulb777 commented Jul 25, 2019

Please fix the travis failure before merging.

@dmandar
Copy link
Contributor Author

@dmandar dmandar commented Jul 25, 2019

Yes, segmentation will have IID and FIS as pod dependencies.

Copy link
Member

@paulb777 paulb777 left a comment

Updating travis.yml is not a good idea since it forces all jobs to run instead of just the ones impacted by the change.

@dmandar dmandar merged commit b3df79f into md-floc-master Jul 25, 2019
1 of 2 checks passed
@morganchen12 morganchen12 deleted the md-floc-1 branch Aug 1, 2019
@firebase firebase locked and limited conversation to collaborators Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants