-
-
Notifications
You must be signed in to change notification settings - Fork 841
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
Location full accuracy (issue #489) #503
Conversation
…curacy # Conflicts: # example/ios/Podfile.lock # ios/RNPermissions.h
I think this will be a very good addition to the package 👍 🎉 One thing I found a bit inconvenient to work with is the way the required/optional rationele are implemented and break current implementations. Example of an implementation that now breaks:
Typescript output:
Any suggestion how to request permissions in an elegant way cross-platform? |
It's great, but we need to find a better API than this one. |
…dd informative error when purposeKey is not specified.
I restored the definition of @zoontek happy to change this around if you have some suggestions. I also agree that it's not going to win any API design awards. |
Another solution would be making a breaking change (3.0.0) and enforcing usage of TypeScript 4 (with labelled tuples - https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/). It would also allow the renaming of checkNotifications / requestNotifications. But this version juste came out, it's a bit extreme. |
@zoontek Are the new labelled tuples any different from function overloads that you can already do in TypeScript? Either way, I think a separate function is better for check/requestNotifications because it's easier to document and understand for users of the package. Function overloads are confusing, as demonstrated by me stuffing up the original version of this PR. I had tried adding a new requestLocationTemporaryFullAccuracy function, but it seemed overkill for this iOS-only beta functionality. The temporary purpose key also seemed to match the "rationale" parameter. If Android had similar precision permissions, with the need for additional options, it might be worth it. |
@adapptor-kurt It can be used like this. The confusion came from the mix-up with the Android rationale, which are not options but messages displayed to the user. |
@zoontek The overload version is like this. I did have it like this, but made a mistake that broke @jaspermeijaard's case. |
temporaryPurposeKey is a reference to a message defined in the plist. You can argue that this is also not an option, but a message displayed to the user. |
@zoontek I'd like to get this in, but if you have reservations about the API then I'm happy to make any suggested changes. I think these are the choices:
|
- (void)requestWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve | ||
rejecter:(void (^ _Nonnull)(NSError * _Nonnull))reject | ||
rationale:(NSDictionary *_Nullable)rationale { | ||
if (!CLLocationManager.locationServicesEnabled || CLLocationManager.authorizationStatus != RNPermissionStatusAuthorized) { |
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.
You can't compare CLLocationManager.authorizationStatus
(Apple API) with RNPermissionStatusAuthorized
(RNPermissions API)
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.
🤦 good spotting, not sure what I was thinking.
RNPermissionStatus status = [RNPermissionHandlerLocationFullAccuracy getAccuracyStatus:locationManager]; | ||
|
||
// Ignore errors due to full accuracy already being authorized | ||
if (error && (error.code != kCLErrorPromptDeclined || status != RNPermissionStatusAuthorized)) { |
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.
Isn't it (error && error.code != kCLErrorPromptDeclined) || status != RNPermissionStatusAuthorized
?
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.
No, a failure to get authorisation should still resolve the promise successfully.
What's happening here is that requestTemporaryFullAccuracyAuthorizationWithPurposeKey will error with kCLErrorPromptDeclined when permission is already granted, causing it not to show a prompt to the user. When we see this error, and can confirm that authorisation is granted, the call is treated as a success instead.
A fourth option is possible: enforcing Later (when iOS 14 will be widely available), it will be nice to rethink the API in a major |
Keep up the good work. I think you guys are best aware of the requirements but defining a rest type based on the permission might allow you to make the second argument required/optional? See this example. |
I don't mind adding a default purpose key, but I already have a use case in an app I'm working on for more than one message.
Ah, I forgot that conditional types are distributive, so this does the right thing 👍 |
I have some changes which I think provide a reasonable level of type safety without making things too complicated. I'll use "full-accuracy" as the default purpose key so that the options are no longer required, and then change the definition to this: function request<T extends Permission = Permission>(
permission: T,
options?: T extends typeof PERMISSIONS.IOS.LOCATION_FULL_ACCURACY
? FullAccuracyOptionsIOS
: Rationale,
): Promise<PermissionStatus>; How's this look? |
…on possible permission types.
@zoontek the code changes are up, just waiting for some feedback before updating documentation. |
This API looks good and doesn't seem to break current permission requests. I'll continue testing when it's more final. |
Do you have a suggestion on how to reference it in an existing project? I tried to modify the |
Try using the branch directly from adapptor-kurt his fork, see NPM docs. |
@creat-or you can try this as suggested by @jaspermeijaard
|
@adapptor-kurt When I check this new permission in an iOS 14.0 simulator of Xcode version 12.0 beta 6, regardless of the value of "Precise Location" in the Settings the check always resolves to
|
Thank you @jaspermeijaard and @toshe! @jaspermeijaard Yes, I have the same behaviour and also wondering if we miss something. |
I think status @creat-or You need to include the pod in your project, please review your |
@jaspermeijaard 🙈 ... awesome, that worked, thank you! :) |
@jaspermeijaard @creat-or Hi! I pulled down from that branch into my project and included the pod, but when I tried to compile shows me the next error. Are you getting the same one?
|
@ezeferex the LocationFullAccuracy pod requires Xcode 12. The GM seed is available from Apple's developer portal. For everyone that's tried it, has it all been working ok? The documentation isn't particularly helpful so I could improve that, but most of these permissions already require you to read a lot of external documentation to understand how to use them. |
@adapptor-kurt Tested on several OS versions with several people and everything seem to work good. Thank you! As far as I am concerned, green light for merging 👍 |
…curacy # Conflicts: # example/ios/Podfile.lock
By when do you think the code will get merged and permissions library would be ready for use with IOS 14 changes? |
@tanishkbajaj Frankly, I don't know. I planned to work on it all day this Saturday, put cannot promise it will be done on time. A lot of things has to be treated: Checking for support from iOS 10 to iOS 14, add support for Android 11 too (and check for support from Android 4.1 to Android 11!), ensure that we don't miss new features that could introduces another required breaking changes later, check that the APIs for all these new features / permissions play well together, write a migration guide, etc. I currently work alone on this, do it on my spare time and for free: even if a lot of companies seems to use this package, I got something like 20$ a month (from a personal friend and my company 😅). Understand me: I don't do OSS for money or glory and I'm not a company with a clear roadmap trying to sell you some service too. That's what open source is 🙂 Sure, I will do my best releasing a beta for this new version as soon as possible. But meanwhile, you can still checkout @adapptor-kurt branch, use https://github.com/ds300/patch-package or even copy and paste new permission handlers directly in your project. |
@tanishkbajaj behold the beauty of patch-package! https://github.com/ds300/patch-package Integrate the PR, and enjoy |
@zoontek I understand your point. Hats off to the hard work you guys put, hope I can contribute too next time. Also meanwhile I can checkout and look at @adapptor-kurt branch. |
I have a problem now with another permission while using react-native-camera. When I use this PR with
With the latest stable version the camera works as expected. |
I can't reproduce this error in the react-native-permissions example app. When options is @creat-or Could you provide some more details about what you're testing with (RN, OS, etc versions). Have you tried a clean build, maybe by also reinstalling your node_modules and pods? |
I managed to reproduce the behavior in a minimal example with the plugins that I use: https://github.com/creat-or/MinimalQRCodeScanner The versions, testing on a real iPhone7 with iOS14 (mac OS 10.15.6, XCode 12)
|
@creat-or your crash is caused by having two copies of react-native-permissions. Yarn is failing to realise that my fork is compatible with the one used by react-native-qrcode-scanner:
When react-native-qrcode-scanner tries to call the API, it's calling the old API with a single parameter but it ends up invoking the pod from the fork that expects two. You can fix this by forcing the resolution in package.json:
If this PR is merged and released with a compatible minor version, then yarn should resolve this properly and the forced resolution shouldn't be necessary. |
Thank you very much for your great assistance! Now the camera permissions work again 👍 Thanks!! |
In case you haven't, see this comment: #503 (comment) |
@adapptor-kurt Sorry for the delay and thanks for the good work. |
Thanks @zoontek! Apple and Google don't make it easy, so your hard work putting everything together and testing is really appreciated. @creat-or since this is going into a v3.0.0, that indicates a breaking change with v2. I would recommend continue using my branch until v3.0.0 is released and |
Thanks for this update! @zoontek: Do you have a rough guess of how long before the 3.0.0 branch might be released? Like, do you expect it to be days, weeks, or months? My team is trying to decide whether to just wait for the official release vs using a branch or fork sooner. Thanks again! |
For those who missed it, I published a first beta of the next major release. Feedbacks are welcomed 🙂 yarn add react-native-permissions@next A migration guide is available here: https://github.com/zoontek/react-native-permissions/blob/3.0.0/MIGRATION.md |
Adds a LocationFullAccuracy pod for checking and requesting (temporary) full accuracy on iOS 14. Addresses issue #489.
Updates the
request
rationale to require atemporaryPurposeKey
for this permission. I experimented with adding a separate function for this temporary permission, but I think this solution is more elegant. Feedback welcome.Test Plan
Test on an iOS 14 beta 3 device in the example app with combinations of when-in-use/background and full/reduced accuracy permissions.
What's required for testing (prerequisites)?
What are the steps to reproduce (after prerequisites)?
Compatibility
Checklist
README.md
CHANGELOG.md
example/App.js
)