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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app): include play service utils #3240

Closed
wants to merge 20 commits into from

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Feb 26, 2020

fixes this issue: https://invertase.canny.io/react-native-firebase/p/playservicesavailability.

Summary

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

Release Plan

[CATEGORY][type] [LOCATION] - Message


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@codecov
Copy link

@codecov codecov bot commented Feb 26, 2020

Codecov Report

Merging #3240 into next will decrease coverage by 0.38%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             next    #3240      +/-   ##
==========================================
- Coverage   85.44%   85.07%   -0.37%     
==========================================
  Files         108      108              
  Lines        3392     3407      +15     
==========================================
  Hits         2898     2898              
- Misses        494      509      +15

@@ -31,6 +31,41 @@ class FirebaseUtilsModule extends FirebaseModule {
return this.native.isRunningInTestLab;
}

get androidPlayServices() {
Copy link
Member

@Salakar Salakar Feb 26, 2020

Choose a reason for hiding this comment

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

This use to be called playServicesAvailability - the example above in the quick start also uses playServicesAvailability

Copy link
Member Author

@russellwheatley russellwheatley Feb 26, 2020

Choose a reason for hiding this comment

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

Thanks, Mike. Sorting out types now 馃憤

packages/app/lib/utils/index.js Outdated Show resolved Hide resolved
packages/app/lib/utils/index.js Outdated Show resolved Hide resolved
packages/app/lib/utils/index.js Outdated Show resolved Hide resolved
packages/app/lib/utils/index.js Outdated Show resolved Hide resolved
packages/app/lib/utils/index.js Outdated Show resolved Hide resolved
Copy link
Member

@Salakar Salakar left a comment

Made a note on a couple small things I noticed, also the TS types are missing here

russellwheatley and others added 6 commits Feb 26, 2020
default `return` for iOS

Co-Authored-By: Mike Diarmid <mike.diarmid@gmail.com>
Co-Authored-By: Mike Diarmid <mike.diarmid@gmail.com>
resolve promises

Co-Authored-By: Mike Diarmid <mike.diarmid@gmail.com>
@Ehesp
Copy link
Member

@Ehesp Ehesp commented Feb 27, 2020

One to document in the new docs - maybe under app?

@russellwheatley
Copy link
Member Author

@russellwheatley russellwheatley commented Feb 27, 2020

@Ehesp Agreed. I've already put the utils under app usage. Once this clears , I'll put in new docs 馃憤

return Promise.resolve(null);
}
return this.native.androidGetPlayServicesStatus();
}

promptForPlayServices() {
if (isIOS) {
return Promise.resolve(null);
}
return this.native.androidPromptForPlayServices();
}

makePlayServicesAvailable() {
if (isIOS) {
return Promise.resolve(null);
}
return this.native.androidMakePlayServicesAvailable();
}

resolutionForPlayServices() {
if (isIOS) {
return Promise.resolve(null);
Copy link
Member

@Salakar Salakar Feb 27, 2020

Choose a reason for hiding this comment

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

Your types now say these return undefined on iOS; updated below to match;

Suggested change
return Promise.resolve(null);
}
return this.native.androidGetPlayServicesStatus();
}
promptForPlayServices() {
if (isIOS) {
return Promise.resolve(null);
}
return this.native.androidPromptForPlayServices();
}
makePlayServicesAvailable() {
if (isIOS) {
return Promise.resolve(null);
}
return this.native.androidMakePlayServicesAvailable();
}
resolutionForPlayServices() {
if (isIOS) {
return Promise.resolve(null);
return Promise.resolve();
}
return this.native.androidGetPlayServicesStatus();
}
promptForPlayServices() {
if (isIOS) {
return Promise.resolve();
}
return this.native.androidPromptForPlayServices();
}
makePlayServicesAvailable() {
if (isIOS) {
return Promise.resolve();
}
return this.native.androidMakePlayServicesAvailable();
}
resolutionForPlayServices() {
if (isIOS) {
return Promise.resolve();

Copy link
Member Author

@russellwheatley russellwheatley Feb 27, 2020

Choose a reason for hiding this comment

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

I'm confused, I thought my types for iOS were returning null 馃槄? If you prefer undefined I'll change accordingly 馃憤

Copy link
Member

@Salakar Salakar Feb 27, 2020

Choose a reason for hiding this comment

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

Types only mention void (undefined), e.g. resolutionForPlayServices(): Promise<void>;:

image

If you change this then the @android docs tag on each probably also needs updating

Copy link
Member Author

@russellwheatley russellwheatley Feb 27, 2020

Choose a reason for hiding this comment

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

Ah, I see. Cool, I'll update to :Promise<void | null> for the types then 馃憤

* If true, allows the device to collect analytical data and send it to
* Firebase. Useful for GDPR.
Copy link
Member

@Salakar Salakar Feb 27, 2020

Choose a reason for hiding this comment

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

Copy pasta derp 馃憖

@@ -379,6 +465,66 @@ export namespace Utils {
* @android
*/
isRunningInTestLab: boolean;

/**
* Returns true if this app is running inside a Firebase Test Lab environment. Always returns false on iOS.
Copy link
Member

@Salakar Salakar Feb 27, 2020

Choose a reason for hiding this comment

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

Copy pasta derp 馃憖

@Salakar Salakar changed the base branch from master to next Mar 3, 2020
@Salakar Salakar added this to the v7.0.0 milestone Mar 3, 2020
@Salakar Salakar added the Type: New Feature label Mar 3, 2020
@mikehardy mikehardy deleted the @russell/map-native-utils-to-js branch Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants