search

FormidableLabs / react-native-app-auth

React native bridge for AppAuth - an SDK for communicating with OAuth2 providers
https://commerce.nearform.com/open-source/react-native-app-auth
MIT License
2.04k stars 441 forks source link

feat(macos): support react-native-macos #1002

Open shirakaba opened 4 months ago

shirakaba commented 4 months ago

Fixes #1001

Description

Adds react-native-macos support 🥳

Steps to verify

Follow the updated setup instructions in examples/demo/README.md to run the demo app. See the following screenshots for expected behaviour with the two providers:

Screenshot 2024-07-22 at 13 16 54

Screenshot 2024-07-22 at 13 17 40

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: b3da68bf6733b1c896eb59768c977dbfd86ec06f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | --------------------- | ----- | | react-native-app-auth | Minor | | rnaa-demo | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-app-auth ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 6:02am
shirakaba commented 4 months ago

Thanks @Saadnajmi for your great guidance! I'll address those aspects once I'm free (think today I'll be working on other things, however). As we discussed on Twitter, react-native-test-app may be the way forwards for integrating an out-of-tree-platform's example app into this repo, and rnx-kit may be able to help out with the Metro config.

shirakaba commented 3 months ago

@Saadnajmi I've now migrated the PR to use react-native-test-app, which resolves my issues with the Metro bundler. I've also aligned to a common minor of React Native, and have addressed all the ifdefs.

However, the demo app requires a custom AppDelegate to make it conform to RNAppAuthAuthorizationFlowManager. How would I set that up?

Saadnajmi commented 3 months ago

@Saadnajmi I've now migrated the PR to use react-native-test-app, which resolves my issues with the Metro bundler. I've also aligned to a common minor of React Native, and have addressed all the ifdefs.

However, the demo app requires a custom AppDelegate to make it conform to RNAppAuthAuthorizationFlowManager. How would I set that up?

I don't think RNTA supports custom App Delegates, so that would be a feature request (@tido64 fyi). It does support expo config plugins, does that let you modify the app delegate? https://github.com/microsoft/react-native-test-app/wiki/Config-Plugins

Though I'm realizing for macOS, that means you need 0.74 😅

shirakaba commented 3 months ago

Unfortunately the one implementing Expo config plugins support for react-native-macos is none other than myself, so I can report that it’s not ready yet 😅

I’ve actually already completed the implementation of Expo config plugins, but need to rewrite the tests, which I was hoping to get at this evening. Still though, it would depend on Expo reviewing and merging it, and even then would only make it into SDK 51 (which expects RN 0.74 🥲) or later. So I think we’d either need a new feature in RNTestApp or go without it in this case.

shirakaba commented 3 months ago

Okay, I've ripped out RNTA again and now have a mix of react-native@0.72.4 and react-native-macos@0.73.30.

This is the best I can do! Would be able to get them both on the same version if RNTA supported custom AppDelegates, however.

tido64 commented 3 months ago

Would be able to get them both on the same version if RNTA supported custom AppDelegates, however.

I don't know if this helps, but I've had this branch for a while that I've just submitted: https://github.com/microsoft/react-native-test-app/pull/2160

shirakaba commented 3 months ago

@tido64 Thanks so much for that PR; it should make everything easier! Sorry I wasn't able to review any closer – I had my hands full last week. I'll try it out once I get back to this PR.

shirakaba commented 3 months ago

@robwalkerco Thank you for the review! Pending tasks in my mind:

I've been a little burnt out lately so may not be able to jump straight on this but do hope to push this over the line soon.

shirakaba commented 3 months ago

I've done a little more work on this to see if this can be migrated to React Native Test App, which would be helpful for adding Windows support in future.

@tido64 I'm struggling to use macos.withAppDelegate() to set up the necessary custom AppDelegate. We need to do two things:

  1. Make AppDelegate conform to RNAppAuthAuthorizationFlowManager. This is possible by referencing the following examples/demo/config-plugin.js via "plugins": ["./config-plugin.js"] in examples/demo/app.json:

    const { createRunOncePlugin, } = require("@expo/config-plugins");
const { mergeContents } = require("@expo/config-plugins/build/utils/generateCode");
const { macos } = require("react-native-test-app/plugins/index");

function withCustomAppDelegate(config) {
return macos.withAppDelegate(config, (config) => {
  // See:
  // - examples/demo/node_modules/react-native-test-app/macos/ReactTestApp/AppDelegate.swift
  // - https://github.com/microsoft/react-native-test-app/pull/2160#issue-2435799831
  config.modResults.contents = mergeContents({
    tag: "config-plugin.js",
    src: config.modResults.contents,
    newSrc: "extension AppDelegate: RNAppAuthAuthorizationFlowManager {}",
    anchor: /extension AppDelegate {/,
    offset: 0,
    comment: "//",
  }).contents;

  return config;
});
}

module.exports = createRunOncePlugin(
withCustomAppDelegate,
"test-plugin.js",
"UNVERSIONED"
);
    1. Express the following Obj-C as Swift in AppDelegate.swift: 
      
#import <React/RCTLinkingManager.h>

    // ... a few lines later:

    • (void)applicationWillFinishLaunching:(NSNotification *)notification { [[NSAppleEventManager sharedAppleEventManager] setEventHandler:self andSelector:@selector(getURL:withReplyEvent:) forEventClass:kInternetEventClass andEventID:kAEGetURL]; }

    • (void)getURL:(NSAppleEventDescriptor )event withReplyEvent:(NSAppleEventDescriptor )reply { NSString urlString = [[event paramDescriptorForKeyword:keyDirectObject] stringValue]; NSURL url = [NSURL URLWithString:urlString];

    if ([self.authorizationFlowManagerDelegate resumeExternalUserAgentFlowWithURL:url]) { return; }

    [RCTLinkingManager getUrlEventHandler:event withReplyEvent:reply]; }

    
... Unfortunately, the current AppDelegate has multiple occurences of the string "applicationWillFinishLaunching", so it is impossible to come up with a non-fragile RegExp pattern to use with it (particularly as the config plugin does not support multi-line regexes). The Expo Config AppDelegate Plugin moreover seems to no-op if there's more than one match.

Largely, this is down to the mergeContents() method from Expo Config Plugins being a very limited API to begin with. I suppose there's no need to use mergeContents(), actually – maybe we can just do a bit of manual string manipulation ourselves inside withAppDelegate 🤔

shirakaba commented 3 months ago

@robwalkerco I'm proceeding to try to get this onto React Native Test App so that adding support for Windows in future will be simpler and to improve long-term maintainability (we'd be able to just bump the version of RNTA to upgrade all example apps). Would you be happy with me migrating the repo to use RNTA as part of this PR, or should it be a follow-up PR?

(Noting that I wasn't having much luck getting this working without RNTA – testing the react-native and react-native-macos example apps required manually flipping a variable in metro.config.js depending on which one wanted to bundle at the given time)

tido64 commented 3 months ago

... Unfortunately, the current AppDelegate has multiple occurences of the string "applicationWillFinishLaunching", so it is impossible to come up with a non-fragile RegExp pattern to use with it (particularly as the config plugin does not support multi-line regexes). The Expo Config AppDelegate Plugin moreover seems to no-op if there's more than one match.

I added some anchor points. I hope this helps: https://github.com/microsoft/react-native-test-app/pull/2188

robwalkerco commented 3 months ago

Would you be happy with me migrating the repo to use RNTA as part of this PR, or should it be a follow-up PR?

@shirakaba It would be preferable if adding RNTA could be handled in a separate PR. That should also help to get this initial macOS PR merged more quickly for you.

shirakaba commented 3 weeks ago

@robwalkerco I've:

image image

Is there any blocker to merging this now?