-
Notifications
You must be signed in to change notification settings - Fork 5
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 common headers for Platform, SDK version, Source app #455
Conversation
📲 You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.
|
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.
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.
Just a small change to a comment. Looks good.
@@ -14,6 +14,9 @@ struct URLSessionHTTPClient: HTTPClient { | |||
let configuration = URLSessionConfiguration.default | |||
configuration.httpAdditionalHeaders = [ | |||
"Accept": "application/json", | |||
"X-Platform": "ios", |
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.
This reminds me of a separate question: do we want to track iOS/iPadOS? If so, I think we'd still treat them both as X-Platform ios
, and add a separate header for device form factor. That way both platforms could record this.
We could add a separate header like "X-Device-Type: tablet"
.
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.
For now we don't want to track iOS/iPadOS and yes i agree that would be a separate header if needed.
let defaultBundle = Bundle(for: BundleFinder.self) | ||
// If installed with CocoaPods, resources will be in GravatarUI.bundle | ||
if let bundleURL = defaultBundle.resourceURL, | ||
let resourceBundle = Bundle(url: bundleURL.appendingPathComponent("Gravatar.bundle")) |
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.
Nice.
For those visiting this in the future:
The name of the bundle "Gravatar.bundle"
(without the .bundle
file extension) needs to match the key in the respective Gravatar.podspec
:
s.resource_bundles = {
'Gravatar' => ['Sources/Gravatar/Resources/*.plist']
}
We happen to be using the same name ("Gravatar") for the module and the pod, too. So it's easier to get right. But the actual dependency is on the name of the resource bundle we define in the podspec.
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 for elaborating. This is valuable info so i added it in the code comment.
extension Bundle { | ||
static var module: Bundle { | ||
let defaultBundle = Bundle(for: BundleFinder.self) | ||
// If installed with CocoaPods, resources will be in GravatarUI.bundle |
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.
// If installed with CocoaPods, resources will be in GravatarUI.bundle | |
// If installed with CocoaPods, resources will be in Gravatar.bundle |
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.
Done.
Closes #451
Description
Adding headers:
X-Platform: ios
X-SDK-Version: <version>
X-Source: <app bundle name>
I created a follow-up issue to find a better way about injecting the SDK version into the iOS runtime. #456
Testing Steps
SwiftUI demo app > QE
Check with a tool like proxyman and confirm the headers are added.