-
Notifications
You must be signed in to change notification settings - Fork 586
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
Functions migration to Kotlin #5351
Conversation
Coverage Report 1Affected Products
Test Logs |
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseFunctions.kt
Outdated
Show resolved
Hide resolved
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.
Left some comments on notable issues
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableOptions.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableOptions.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableResult.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableResult.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableResult.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallOptions.kt
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableReference.kt
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseFunctions.kt
Outdated
Show resolved
Hide resolved
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.
Some key things I was seeing to look out for:
package-private
members- public properties (that had public getters or setters) need
@JvmField
to prevent a breaking change
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableOptions.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableReference.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableOptions.kt
Outdated
Show resolved
Hide resolved
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.
I added some of my thoughts/suggestions/wishes for the API surface.
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableOptions.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableResult.kt
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/HttpsCallableReference.kt
Outdated
Show resolved
Hide resolved
firebase-functions/src/main/java/com/google/firebase/functions/FirebaseFunctions.kt
Outdated
Show resolved
Hide resolved
Size Report 1Affected Products
Test Logs |
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.
The test failures are valid (esp the unit tests for functions). I'll see if I can't personally take a look later
Took a look and I can't repro the unit/integ test failures locally, wondering what's causing the failure in CI |
8433e1f
to
c658223
Compare
The public api surface has changed for the subproject firebase-functions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
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.
So long as the smoke test is a flake, LGTM
Release note changesThe following release notes were modified. Please ensure they look correct. Release Notesfirebase-functions### {{functions_client}} version 21.1.0 {: #functions-client_v21-1-0}
* {{changed}} Migrated to Kotlin
#### {{functions_client}} Kotlin extensions version 21.1.0 {: #functions-client-ktx_v21-1-0}
The Kotlin extensions library transitively includes the updated
`firebase-functions` library. The Kotlin extensions library has no additional
updates. |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
The public api surface has changed for the subproject firebase-functions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
done with automated tooling, to investigate the viability of quick migrations pre API changes.