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

feat: External Link Native Module #2107

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jimhunty
Copy link
Contributor

@jimhunty jimhunty commented Sep 15, 2022

Why are you doing this?

With the release of iOS16, and with our progression towards Reader App status, this module hopes to expose the Native External Link module for iOS. A noop is created for Android to give parity between the platforms

Changes

  • Android native module added with zero functionality
  • iOS Native module added with an attempt to access External Link
  • TS function added to expose these modules

Please note: This is untested in iOS due to the inability to build (Ophan issue). However, it builds and works as expected for Android.

Screenshots

Simulator Screen Shot - iPad Pro (12 9-inch) (5th generation) - 2022-10-17 at 08 37 27
Simulator Screen Shot - iPad Pro (12 9-inch) (5th generation) - 2022-10-17 at 08 37 06

@jimhunty jimhunty requested a review from a team September 15, 2022 07:20
Copy link
Contributor

@groakland groakland left a comment

Choose a reason for hiding this comment

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

LGTM

@jimhunty jimhunty force-pushed the feat/external-link-native-module branch from d11c984 to 01b48fc Compare September 22, 2022 06:43
@jimhunty jimhunty force-pushed the feat/external-link-native-module branch from 91c3e0c to edf049b Compare September 29, 2022 06:49
Copy link

@ddramowicz ddramowicz left a comment

Choose a reason for hiding this comment

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

Added a couple ideas around how to get Swift async/await to work with RN.
The other option could be to open the modal completely natively, that way you wouldn't need to ever wait for a callback from the native layer before opening the modal from RN.
The RN layer would call a nativeopen function of some kind, and it would do all the logic in there, e.g.

  @objc
    func open() {
        guard #available(iOS 16.0, *) else { return }
        Task {
            let canOpen = await ExternalLinkAccount.canOpen
            let canMakePayments = SKPaymentQueue.canMakePayments()

            if canOpen, canMakePayments {
               try await ExternalLinkAccount.open()
            }
        }
    }

Even nicer would be to make this call throwing so we could pass back any failures to the RN layer (if RN knows how to handle throwing methods, I don't know!)

if #available(iOS 16.0, *) {
Task {
let result = try await ExternalLinkAccount.canOpen
return result

Choose a reason for hiding this comment

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

I don't think this result is being passed back or stored anywhere?

projects/Mallard/src/helpers/external-link.ts Show resolved Hide resolved
@objc func canOpen() {
if #available(iOS 16.0, *) {
Task {
let result = try await ExternalLinkAccount.canOpen

Choose a reason for hiding this comment

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

you also don't need to try here as canOpen is not a throwing function

return result
}
}
}

Choose a reason for hiding this comment

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

Well, the above comments are more FYI rather than something that will actually help with your problem of interfacing React Native with Swift's Async/Await. I found this SO post which suggested using events (https://stackoverflow.com/questions/72302077/react-native-module-in-swift-with-async-function-throws-is-not-a-recognized-obje), would that be an option, do you think? Something like this:

  @objc
    func canOpen() {
        guard #available(iOS 16.0, *) else { return }
        Task {
            let result = await ExternalLinkAccount.canOpen
            sendEvent(withName: "OpenEvent", body: result)
        }
    }

I'm not sure how you'd get the React layer to await the result from the event, but maybe this is an option? It sounds like Swift async functions don't work in React Native, which is a shame as many new iOS APIs are Swift-only and often use async/await...

@jimhunty
Copy link
Contributor Author

Thanks for your feedback @ddramowicz, thats all very helpful. I have gone with the simpler solution suggested. The only downside is the RN layer not knowing when it fails, but this should be edge case and something we can look to improve in future.

@ddramowicz
Copy link

Nice! Looks good, but we should probably hold off on merging until we can test it - we'll need that entitlement as well for this to work as expected.

@jimhunty
Copy link
Contributor Author

jimhunty commented Nov 3, 2022

Waiting on legal sign off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants