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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ios][local-authentication] Migrate module to Sweet API #19980

Merged
merged 5 commits into from Nov 11, 2022

Conversation

fobos531
Copy link
Contributor

Why

Continuation of migration of expo modules to the new API

How

  • routine work (update podspec, module config etc.)
  • 1:1 migration from ObjC to Swift

Test Plan

Tested NCL examples on iPhone 14 Pro & iPhone SE 3rd gen (both simulators). Seems to work fine, with one remark:

  • on iPhone 14 Pro authenticating without device fallback and while not enrolled returns a "usage_description" error even though NSFaceIDUsageDescription is defined in Info.plist. Is this the intended behavior, given this is a simulator?

With that said, I think it would be beneficial if you could test this on other devices (physical & simulator) to verify the correctness of the implementation.

I'd greatly appreciate any comments on the general code quality! A few comments that I have:

  • I'm currently dispatching every function on the main queue. I know there's been some discussing about queues/functions in [android][store-review] Migrate expo-store-review to the new native m… #19898, but I'm not sure I entirely understand the purpose of queues, so I might have used it needlessly - maybe this is something that could be documented in the module guide? i.e. the purpose of queues, when a given function should be dispatched on a given queue (and the reasoning behind it) etc. I think it would help avoid the potential issues people could run into when developing native modules. The AsyncFunction docs already do touch on this matter, but additional clarifications are definitely welcome.

Checklist

@expo-bot expo-bot added the bot: needs changes ExpoBot found things that don't meet our guidelines label Nov 10, 2022
var err: String?

if error != nil {
err = convertErrorCode(error: error as! NSError)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the recent comment got lost, so reiterating here: would appreciate pointers on how to improve the safety of this piece of code

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

on iPhone 14 Pro authenticating without device fallback and while not enrolled returns a "usage_description" error even though NSFaceIDUsageDescription is defined in Info.plist. Is this the intended behavior, given this is a simulator?

I think that's not correct behavior, but looks like your implementation is the same as before, so I'll just take a look after merging this.

I'm not sure I entirely understand the purpose of queues, so I might have used it needlessly - maybe this is something that could be documented in the module guide? i.e. the purpose of queues, when a given function should be dispatched on a given queue (and the reasoning behind it)

The .main queue refers to DispatchQueue.main. It's the queue that runs on the main (UI) thread and where all the UI work is done. Basically when you're doing something UI-related (updating views or view controllers) then you must dispatch it on that queue, otherwise things may crash or have unexpected behavior. Maybe this article will help: https://www.donnywals.com/appropriately-using-dispatchqueue-main
Good point that we should note that in the docs, thanks!
In case of local authentication, it seems that we don't need to run those functions on the main thread and in the old implementation everything is run on the background thread — it doesn't override - (dispatch_queue_t)methodQueue. That being said, I think you can remove .runOnQueue(.main) from all these functions 😉

Could you also reindent all Swift files? 😅 We prefer using 2 spaces 🙂

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: needs changes ExpoBot found things that don't meet our guidelines labels Nov 11, 2022
@fobos531
Copy link
Contributor Author

Hey @tsapeta , thanks for the awesome and thorough explanation, much appreciated!

I've applied the code changes you suggested 🚀

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

It's perfect now 🚀 Thank you so much for your help 🙏
I'll wait for the CI and then merge 😉

@tsapeta tsapeta merged commit 7b874f3 into expo:main Nov 11, 2022
@fobos531 fobos531 deleted the ios-local-auth-sweet branch November 12, 2022 09:47
@lexer
Copy link

lexer commented Mar 3, 2023

@fobos531 @tsapeta I believe this PR introduced a bug and Face ID no longer working on some devices.

@lexer
Copy link

lexer commented Mar 3, 2023

Tried to report it here. #21494

@lexer
Copy link

lexer commented Mar 3, 2023

Try to call:

    return await LocalAuthentication.authenticateAsync({
      promptMessage: 'test',
      disableDeviceFallback: true,
      requireConfirmation: true,
      fallbackLabel: '',
      cancelLabel: '',
    });

iOS version: 16.2
Model: iPhone 12 mini

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants