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.

Sign up for GitHub

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

Jump to bottom

feat(ios): AEM logger #165

Merged
merged 2 commits into from Jan 2, 2022
Merged

feat(ios): AEM logger #165

merged 2 commits into from Jan 2, 2022

Conversation

rnike
Copy link
Collaborator

@rnike rnike commented Dec 29, 2021

close #153

Aggregated Event Measurement for Facebook App Events

  1. fix project file issue in project.pbxproj (not a serious issue, it's just cannot open some files with xcode when developing this project)
  2. create FBAEMReporter.logAEMEvent method
  3. add a section for AEM in README.md
    a. The official document told us to Add the following to your Podfile: pod 'FBAEMKit', but in our case, FBAEMKit is already a dependency for FBSDKCoreKit, so I didn't mention it in README.md

@rnike
feat: AEM logger
0d76349 
* fix project.pbxproj

* implement logAEMEvent for iOS

* documentation for AEM
@rnike rnike changed the title feat: AEM logger feat!: AEM logger Dec 29, 2021
@rnike rnike mentioned this pull request Dec 29, 2021
Closed
4 tasks
thebergamo
thebergamo reviewed Dec 30, 2021
View reviewed changes
Copy link
Owner

@thebergamo thebergamo left a comment

Choose a reason for hiding this comment

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

Besides my comment it LGTM

src/FBAEMReporter.ts
* @ref https://developers.facebook.com/docs/app-events/guides/aggregated-event-measurement/
* @platform iOS
*/
logAEMEvent: (
Copy link
Owner

Choose a reason for hiding this comment

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

Reviewing from mobile, but isn't it only iOS available?
In case it won't be ported to android in near future, I would make it platform specific as we did for other APIs instead of fallback in android, just for sake of consistency.

In case we do have an android version of it and it's just a case of porting it I would make it clear that support for Android would come soon and/or we're open for contributors :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's iOS only feature. I'm not sure what you mean by "make it platform specific instead of fallback in android", I thought marking it as @platform iOS is what we do for platform specific, the actual use case, as far as I know, is to send an additional event using logAEMEvent

  AppEventsLogger.logEvent(eventName, value, parameters);
  AEMReporter.logAEMEvent(eventName, value, currency, parameters);

The early return on the none-ios platform is to make the function usage easier, so we don't have to do something as below

  AppEventsLogger.logEvent(eventName, value, parameters);
  if(Platform.OS === 'ios'){
    AEMReporter.logAEMEvent(eventName, value, currency, parameters);
  }

I don't know if there will be an android version of AEM, not mentioned on the document, but I think it will not be happening in the near future.

Copy link
Owner

Choose a reason for hiding this comment

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

I got your point, problem is that someone implementing it could be confused and expect that it would works for android as well.

https://developers.facebook.com/docs/facebook-login/limited-login/ios

Take a look in the above part in our Readme.md

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I meant this method: loginTrackingIOS

NB!: the iOS app doesn't help in copy/paste some internal links :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taking your advice, I renamed it to AEMReporterIOS.logAEMEvent

mikehardy
mikehardy approved these changes Dec 31, 2021
View reviewed changes
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks good to me @thebergamo if this looks good to you, then if you merge it will go into the beta branch (I set the target to beta), and then running the release action on the beta branch target will generate another beta I believe

Once both have "marinated" (this one, and typescript) we can then do a PR from beta to master and do a real v7 release ?

@mikehardy mikehardy changed the title feat!: AEM logger feat(ios): AEM logger Dec 31, 2021
@thebergamo thebergamo merged commit de72377 into thebergamo:beta Jan 2, 2022
@thebergamo
Copy link
Owner

Seems great now, merged o/

github-actions bot pushed a commit that referenced this pull request Jan 2, 2022
@semantic-release-bot
chore(release): 7.0.0-beta.2 [skip ci]
f497b22 
## [7.0.0-beta.2](v7.0.0-beta.1...v7.0.0-beta.2) (2022-01-02)

### Features

* **ios:** AEM logger ([#165](#165)) ([de72377](de72377))
@github-actions
Copy link

github-actions bot commented Jan 2, 2022

🎉 This PR is included in version 7.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

mikehardy pushed a commit that referenced this pull request Jan 21, 2022
@rnike @mikehardy
feat(ios): AEM logger (#165)
be1e861
mikehardy pushed a commit that referenced this pull request Jan 21, 2022
@semantic-release-bot @mikehardy
chore(release): 7.0.0-beta.2 [skip ci]
db1b65a 
## [7.0.0-beta.2](v7.0.0-beta.1...v7.0.0-beta.2) (2022-01-02)

### Features

* **ios:** AEM logger ([#165](#165)) ([de72377](de72377))
github-actions bot pushed a commit that referenced this pull request Jan 21, 2022
@semantic-release-bot
chore(release): 7.0.0 [skip ci]
3d78c89 
## [7.0.0](v6.2.0...v7.0.0) (2022-01-21)

### ⚠ BREAKING CHANGES

* **release:** use conventional commits style for commit parsing
* update dependencies
* migrate to typescript (#155)

### Features

* add clearUserID to AppEventsLogger (`setUserId(null)` throws exception from SDK) ([#185](#185)) ([03306a6](03306a6))
* export all module types ([#173](#173)) ([bd7f968](bd7f968))
* **ios:** AEM logger ([#165](#165)) ([be1e861](be1e861))

### Bug Fixes

* **android, build:** delete jcenter from rootProject - should not affect consuming apps ([#184](#184)) ([a76c285](a76c285))
* **android, deprecation:** Replace deprecated GameRequestDialog ([#172](#172)) ([a0c20d2](a0c20d2))
* **android:** remove dead code ([#176](#176)) ([a24bdb4](a24bdb4))
* **iOS:** update deprecated methods ([81fc259](81fc259))
* **release:** use conventional commits style for commit parsing ([c311031](c311031)), closes [#164](#164)
* update dependencies ([ddc968f](ddc968f))

### Code Refactoring

* migrate to typescript ([#155](#155)) ([d55e203](d55e203))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thebergamo thebergamo thebergamo left review comments

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rnike @thebergamo @mikehardy