Skip to content

Add core support with interop for Segmentation SDK. - #3430

Merged
dmandar merged 10 commits into
md-floc-masterfrom
md-floc-1
Jul 25, 2019
Merged

Add core support with interop for Segmentation SDK. #3430
dmandar merged 10 commits into
md-floc-masterfrom
md-floc-1

Conversation

@dmandar

@dmandar dmandar commented Jul 24, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread Firebase/Core/FIRErrors.m Outdated
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread Firebase/Core/Private/FIRErrors.h Outdated
extern NSString *const kFirebaseConfigErrorDomain;
extern NSString *const kFirebaseCoreErrorDomain;
extern NSString *const kFirebasePerfErrorDomain;
extern NSString *const kFirebaseSegmentationErrorDomain;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete. Same as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

#import "FIRAppInternal.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

#import "FIRAppInternal.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use module import format here too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.


@end

/// A concrete implementation for FIRSegmentationInterop to create Segmentation instances and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a need for FIRSegmentationInterop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Comment thread FirebaseSegmentation.podspec Outdated
s.public_header_files = 'FirebaseSegmentation/Public/*.h'
s.public_header_files = 'FirebaseSegmentation/Sources/Public/*.h'

s.dependency 'FirebaseCore', '~> 6.0'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

6.1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dmandar
dmandar requested a review from paulb777 July 25, 2019 05:40
Comment thread FirebaseSegmentation/Sources/FIRSegmentationComponent.m Outdated

@end

/// A concrete implementation for FIRSegmentationInterop to create Segmentation instances and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
Copy Markdown
Member

Please fix the travis failure before merging.

@dmandar

dmandar commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

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

@paulb777 paulb777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
@morganchen12
morganchen12 deleted the md-floc-1 branch August 1, 2019 20:57
@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

Development

Successfully merging this pull request may close these issues.

3 participants