[TM] add spec for PermissionsAndroid #24886

krizzu · 2019-05-16T09:12:32Z

Summary

Part of #24875

Changelog

[General] [Added] - Add TurboModule spec for PermissionsAndroid

Test Plan

Flow passes on root level

retyui · 2019-05-16T09:49:27Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -20,7 +21,7 @@ export type Rationale = {
  buttonNeutral?: string,
 };

-type PermissionStatus = 'granted' | 'denied' | 'never_ask_again';
+type PermissionStatus = 'granted' | 'denied' | 'never_ask_again' | string;


Why are you adding string ? @krizzu

We have had exact declared 3 types:

react-native/ReactAndroid/src/main/java/com/facebook/react/modules/permissions/PermissionsModule.java

Lines 39 to 41 in 803e993

private final String GRANTED = "granted";

private final String DENIED = "denied";

private final String NEVER_ASK_AGAIN = "never_ask_again";

@retyui I've added it because of this method resolve with PermissionStatus. I'm not quite sure if I could add literals in Spec, so I ended up with adding string to the type.

@krizzu
Now Spec is source of truth

You should move PermissionStatus type into Libraries/PermissionsAndroid/NativePermissionsAndroid.js

@retyui
Gotcha. Done that.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot · 2019-05-16T18:12:35Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -11,6 +11,9 @@
 'use strict';

 const NativeModules = require('../BatchedBridge/NativeModules');
+import NativePermissionsAndroid from './NativePermissionsAndroid';
+
+import type {PermissionStatus} from './NativePermissionsAndroid'


prettier/prettier: Insert ;

analysis-bot · 2019-05-16T18:12:35Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -11,6 +11,9 @@
 'use strict';

 const NativeModules = require('../BatchedBridge/NativeModules');
+import NativePermissionsAndroid from './NativePermissionsAndroid';
+
+import type {PermissionStatus} from './NativePermissionsAndroid'


semi: Missing semicolon.

RSNara

This is great! Thanks. 😁

retyui · 2019-05-16T18:52:56Z

Libraries/PermissionsAndroid/NativePermissionsAndroid.js

+    permission: string,
+  ) => Promise<boolean>;
+
+  +requestMultiplePermissions: (permissions: Array<string>) => Promise<Object>;


Promise<Object> => Promise<{[permission: string]: PermissionStatus}>

retyui · 2019-05-16T19:05:24Z

@RSNara, @krizzu

How about making more strict permission?

See tests case in the end

krizzu · 2019-05-16T21:04:54Z

@retyui I'm up for it. Moved permissions to NativeAndroidPermissions, where I export both types and values.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot · 2019-05-16T21:06:11Z

Libraries/PermissionsAndroid/NativePermissionsAndroid.js

+ * @flow
+ */
+
+'use strict';


prettier/prettier: Delete ⏎

analysis-bot · 2019-05-16T21:06:11Z

Libraries/PermissionsAndroid/NativePermissionsAndroid.js

+});
+
+export type PermissionStatus = $Values<typeof PERMISSION_REQUEST_RESULT>;
+export type PermissionType = $Values<typeof PERMISSIONS>;


prettier/prettier: Insert ;

analysis-bot · 2019-05-16T21:06:11Z

Libraries/PermissionsAndroid/NativePermissionsAndroid.js

+
+export type PermissionStatus = $Values<typeof PERMISSION_REQUEST_RESULT>;
+export type PermissionType = $Values<typeof PERMISSIONS>;
+


prettier/prettier: Insert ;

analysis-bot · 2019-05-16T21:06:12Z

Libraries/PermissionsAndroid/NativePermissionsAndroid.js

+  ) => Promise<boolean>;
+
+  +requestMultiplePermissions: (
+    permissions: Array<PermissionType>,


prettier/prettier: Replace permissions:·Array<PermissionType>)·=>·Promise<{[permission:·PermissionType]:·PermissionStatus}> with ⏎····permissions:·Array<PermissionType>,⏎··)·=>·Promise<{[permission:·PermissionType]:·PermissionStatus}>;

analysis-bot · 2019-05-16T21:06:12Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -11,6 +11,15 @@
 'use strict';

 const NativeModules = require('../BatchedBridge/NativeModules');
+import NativePermissionsAndroid, {


prettier/prettier: Replace PERMISSION_REQUEST_RESULT,·PERMISSIONS with ⏎··PERMISSION_REQUEST_RESULT,⏎··PERMISSIONS,⏎

analysis-bot · 2019-05-16T21:06:12Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -11,6 +11,15 @@
 'use strict';

 const NativeModules = require('../BatchedBridge/NativeModules');
+import NativePermissionsAndroid, {
+  PERMISSION_REQUEST_RESULT,
+  PERMISSIONS,


prettier/prettier: Replace PermissionStatus,·PermissionType}·from·'./NativePermissionsAndroid' with ⏎··PermissionStatus,⏎··PermissionType,⏎}·from·'./NativePermissionsAndroid';

analysis-bot · 2019-05-16T21:06:13Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -11,6 +11,15 @@
 'use strict';

 const NativeModules = require('../BatchedBridge/NativeModules');
+import NativePermissionsAndroid, {
+  PERMISSION_REQUEST_RESULT,
+  PERMISSIONS,


semi: Missing semicolon.

krizzu · 2019-05-17T09:24:55Z

Another question: This module is Android only. Would TurboModuleRegistry.getEnforcing<Spec>('PermissionsAndroid') throw/return null on iOS then?

ericlewis · 2019-05-17T15:17:53Z

It would return null, so you may need to wait for that to land to use it, or go ahead and protect call sites perhaps. Sent via Superhuman iOS ( https://sprh.mn/?vip=ericlewis777@gmail.com )

…

On Fri, May 17 2019 at 5:25 AM, < ***@***.*** > wrote: Another question: This module is Android only. Would TurboModuleRegistry.getEnforcing<Spec>('PermissionsAndroid') throw/return null on iOS then? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub ( #24886?email_source=notifications&email_token=AAFEVR4NQT2HWSAXLD43KX3PVZ2XLA5CNFSM4HNKQI6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVUH3WA#issuecomment-493387224 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAFEVR63ROP4CNWQ46ODEB3PVZ2XLANCNFSM4HNKQI6A ).

krizzu · 2019-05-19T11:44:18Z

@ericlewis You think that checking for the platform at the call site and returning negatively when it's not Android?

ericlewis · 2019-05-19T17:16:31Z

Correct. Sent via Superhuman iOS ( https://sprh.mn/?vip=ericlewis777@gmail.com )

…

On Sun, May 19 2019 at 7:44 AM, < ***@***.*** > wrote: @ericlewis ( https://github.com/ericlewis ) You think that checking for the platform at the call site and returning negatively when it's Android? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #24886?email_source=notifications&email_token=AAFEVRZCKEGMUBVZGJRGWQLPWE4SBA5CNFSM4HNKQI6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVXAMUQ#issuecomment-493749842 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAFEVRY2FT3FOH65V5VYSADPWE4SBANCNFSM4HNKQI6A ).

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot · 2019-05-19T18:44:03Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -77,11 +48,13 @@ class PermissionsAndroid {
   *
   * @deprecated
   */
-  checkPermission(permission: string): Promise<boolean> {
+  checkPermission(permission: PermissionType): Promise<boolean> {
    console.warn(
      '"PermissionsAndroid.checkPermission" is deprecated. Use "PermissionsAndroid.check" instead',
    );


curly: Expected { after 'if' condition.

analysis-bot · 2019-05-19T18:44:04Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -90,8 +63,9 @@ class PermissionsAndroid {
   *
   * See https://facebook.github.io/react-native/docs/permissionsandroid.html#check
   */
-  check(permission: string): Promise<boolean> {


curly: Expected { after 'if' condition.

analysis-bot · 2019-05-19T18:44:04Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -109,12 +83,14 @@ class PermissionsAndroid {
   * @deprecated
   */
  async requestPermission(
-    permission: string,
+    permission: PermissionType,
    rationale?: Rationale,
  ): Promise<boolean> {


curly: Expected { after 'if' condition.

analysis-bot · 2019-05-19T18:44:04Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -126,11 +102,13 @@ class PermissionsAndroid {
   * See https://facebook.github.io/react-native/docs/permissionsandroid.html#request
   */
  async request(


curly: Expected { after 'if' condition.

analysis-bot · 2019-05-19T18:44:04Z

Libraries/PermissionsAndroid/PermissionsAndroid.js

@@ -158,11 +134,11 @@ class PermissionsAndroid {
   * See https://facebook.github.io/react-native/docs/permissionsandroid.html#requestmultiple
   */
  requestMultiple(
-    permissions: Array<string>,


curly: Expected { after 'if' condition.

fkgozali

Sorry for the confusion, but the typing should not add any cost to the JS bundle size. That said, let's just define union types inline, instead of defining a frozen object:

Instead of:

export const PERMISSION_REQUEST_RESULT = Object.freeze({
  GRANTED: 'granted',
  DENIED: 'denied',
  NEVER_ASK_AGAIN: 'never_ask_again',
});

export type PermissionStatus = $Values<typeof PERMISSION_REQUEST_RESULT>;

do:

export type PermissionStatus = 'granted' | 'denied' | 'never_ask_again';

Also, can you run prettier formatter?

retyui · 2019-05-20T21:31:39Z

@fkgozali
you should carefully look at PR

fkgozali · 2019-05-20T21:41:50Z

you should carefully look at PR

Cool, but for the purpose for using Flow type to generate the C++/ObjC/Java code, we need the type to be fully inline for enum (can't use $Values<> atm), so we'll have to create the types without modifying the usage of PERMISSION_REQUEST_RESULT.

export type PermissionStatus = 'granted' | 'denied' | 'never_ask_again';

retyui · 2019-05-21T06:17:22Z

@fkgozali

purpose for using Flow type to generate the C++/ObjC/Java code

I can get acquainted with your native code generator?

krizzu · 2019-05-23T14:38:18Z

@fkgozali @retyui
Right, so this means we have to drop $Values<>, and inline those string values instead.

I'm getting into it.

fkgozali · 2019-05-23T15:33:48Z

to simplify, you could just use string type for the enums for the time being

facebook-github-bot

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-05-30T01:35:06Z

This pull request was successfully merged by @krizzu in 82fe1b0.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: Part of facebook#24875 ## Changelog [General] [Added] - Add TurboModule spec for PermissionsAndroid Pull Request resolved: facebook#24886 Reviewed By: RSNara Differential Revision: D15542996 Pulled By: fkgozali fbshipit-source-id: cab02d97e70d65347f63e891cff98c17adc1fdba

[TM] add spec for PermissionsAndroid

cc751d1

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels May 16, 2019

react-native-bot added API: PermissionsAndroid Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels May 16, 2019

retyui reviewed May 16, 2019

View reviewed changes

moves PermissionType to Spec file

eba4024

analysis-bot suggested changes May 16, 2019

View reviewed changes

RSNara approved these changes May 16, 2019

View reviewed changes

retyui reviewed May 16, 2019

View reviewed changes

makes types more strict

68d987c

krizzu force-pushed the spec-androidpermissions branch from 78d5707 to 68d987c Compare May 16, 2019 21:05

analysis-bot suggested changes May 16, 2019

View reviewed changes

ericlewis added the Flow label May 16, 2019

analysis-bot suggested changes May 19, 2019

View reviewed changes

adds check for platform

0e0e926

krizzu force-pushed the spec-androidpermissions branch from 7e82647 to 0e0e926 Compare May 19, 2019 18:51

hramos added the Native Module label May 20, 2019

fkgozali requested changes May 20, 2019

View reviewed changes

Krzysztof Borowy added 2 commits May 23, 2019 21:12

use inline string types

bc1b5eb

adds warning for other platforms

e20c745

facebook-github-bot reviewed May 29, 2019

View reviewed changes

facebook-github-bot closed this in 82fe1b0 May 30, 2019

react-native-bot added the Merged This PR has been merged. label May 30, 2019

	private final String GRANTED = "granted";
	private final String DENIED = "denied";
	private final String NEVER_ASK_AGAIN = "never_ask_again";


		export type PermissionStatus = $Values<typeof PERMISSION_REQUEST_RESULT>;
		export type PermissionType = $Values<typeof PERMISSIONS>;

[TM] add spec for PermissionsAndroid #24886

[TM] add spec for PermissionsAndroid #24886

Conversation

krizzu commented May 16, 2019 • edited Loading

Summary

Changelog

Test Plan

retyui May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizzu May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retyui commented May 16, 2019

krizzu commented May 16, 2019

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizzu commented May 17, 2019

ericlewis commented May 17, 2019 via email

krizzu commented May 19, 2019 • edited Loading

ericlewis commented May 19, 2019 via email

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkgozali left a comment • edited Loading

Choose a reason for hiding this comment

retyui commented May 20, 2019 • edited Loading

fkgozali commented May 20, 2019

retyui commented May 21, 2019

krizzu commented May 23, 2019 • edited Loading

fkgozali commented May 23, 2019 via email • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented May 30, 2019

krizzu commented May 16, 2019 •

edited

Loading

retyui May 16, 2019 •

edited

Loading

krizzu May 16, 2019 •

edited

Loading

RSNara left a comment •

edited

Loading

krizzu commented May 19, 2019 •

edited

Loading

fkgozali left a comment •

edited

Loading

retyui commented May 20, 2019 •

edited

Loading

krizzu commented May 23, 2019 •

edited

Loading

fkgozali commented May 23, 2019 via email •

edited

Loading