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

[facebook][android] Rewrite facebook module to Kotlin #14572

Merged
merged 13 commits into from
Oct 26, 2021
Merged

[facebook][android] Rewrite facebook module to Kotlin #14572

merged 13 commits into from
Oct 26, 2021

Conversation

mstach60161
Copy link
Contributor

Why

Java modules are rewritten to kotlin.

How

  • Convert java code to Kotlin
  • Clean up code

Test Plan

  • Test suite

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 29, 2021
@lukmccall lukmccall changed the title [facebook][android] Rewrite filesystem module to Kotlin [facebook][android] Rewrite facebook module to Kotlin Sep 30, 2021
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 30, 2021
barthap
barthap reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@barthap barthap left a comment

Choose a reason for hiding this comment

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

Nice! A few suggestions on how I'd do that, but they're up to you

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
Comment on lines 172 to 178
if (FacebookSdk.getApplicationId() == null) {
promise.reject(
ERR_FACEBOOK_MISCONFIGURED,
"No appId configured, required for initialization. " +
"Please ensure that you're either providing `appId` to `initializeAsync` as an argument or inside AndroidManifest.xml."
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR, but shouldn't the method return here as the promise is rejected anyway? I see the return was missing in Java too. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

We for sure should add the return here. I guess anyone noticed that cause on Android, you can resolve promise multiple times - what btw is stupid 🤷‍♂️

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookPackage.kt Outdated Show resolved Hide resolved
Michał Stach added 2 commits September 30, 2021 15:43
lukmccall
lukmccall reviewed Sep 30, 2021
View reviewed changes
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
Comment on lines 172 to 178
if (FacebookSdk.getApplicationId() == null) {
promise.reject(
ERR_FACEBOOK_MISCONFIGURED,
"No appId configured, required for initialization. " +
"Please ensure that you're either providing `appId` to `initializeAsync` as an argument or inside AndroidManifest.xml."
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We for sure should add the return here. I guess anyone noticed that cause on Android, you can resolve promise multiple times - what btw is stupid 🤷‍♂️

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
@ExpoMethod
fun getAdvertiserIDAsync(promise: Promise) {
try {
promise.resolve(attributionIdentifiers!!.androidAdvertiserId)
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

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 think we reject in catch block (same in suggestions below) but if it's bad practise (?) , I can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, you are right, but I don't like this. I think we should avoid using the try-catch block. It'll look better that way in my opinion. Also, it's more self-explanatory.

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
@ExpoMethod
fun getAttributionIDAsync(promise: Promise) {
try {
promise.resolve(attributionIdentifiers!!.attributionId)
Copy link
Contributor

Choose a reason for hiding this comment

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

here too

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
@ExpoMethod
fun flushAsync(promise: Promise) {
try {
appEventLogger!!.flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

reject 🤔

packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookPackage.kt Outdated Show resolved Hide resolved
barthap
barthap reviewed Sep 30, 2021
View reviewed changes
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated
Comment on lines 35 to 38
private val ERR_FACEBOOK_MISCONFIGURED = "ERR_FACEBOOK_MISCONFIGURED"
private val ERR_FACEBOOK_LOGIN = "ERR_FACEBOOK_LOGIN"
private val PUSH_PAYLOAD_KEY = "fb_push_payload"
private val PUSH_PAYLOAD_CAMPAIGN_KEY = "campaign"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @lukmccall meant to move these constants outside class definition 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, exactly 😅

@barthap barthap mentioned this pull request Oct 1, 2021
Merged
kkafar
kkafar reviewed Oct 1, 2021
View reviewed changes
Copy link
Contributor

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Looks good

packages/expo-facebook/android/build.gradle Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Outdated Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookModule.kt Show resolved Hide resolved
packages/expo-facebook/android/src/main/java/expo/modules/facebook/FacebookPackage.kt Outdated Show resolved Hide resolved
bbarthec
bbarthec approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

As for me it looks fine 👍
This module might not be the most elegant one, but the conversion looks fine.
Refer to the comments from others and we're ready to ship it 🚢

Michał Stach added 2 commits October 14, 2021 19:21
@bbarthec
Copy link
Contributor

@Jsjkwnw, if you're facing some issue, create an actual Issue describing your problem. I've deleted your comments as these are completely unrelated to this PR and pollute the work being processed here.

@expo expo deleted a comment from Jsjkwnw Oct 26, 2021
@expo expo deleted a comment from Jsjkwnw Oct 26, 2021
@bbarthec bbarthec merged commit 30432ba into expo:master Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkafar kkafar kkafar left review comments

@lukmccall lukmccall lukmccall left review comments

@bbarthec bbarthec bbarthec approved these changes

@barthap barthap barthap approved these changes

Assignees
No one assigned
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mstach60161 @bbarthec @barthap @lukmccall @kkafar @expo-bot