feat: add new arch by WoLewicki · Pull Request #557 · plaid/react-native-plaid-link-sdk

WoLewicki · 2023-09-20T16:08:19Z

Summary

PR adding new arch to the library. It should no longer be needed to do any autolinking since it is done automatically and TurboModule is used when they are enabled in the project.

Motivation

It makes the library compatible with new architecture, so users can take advantage of it.

📝 Checklist

I have performed a self-review of my own code.
I have optimized code readability (class/variable names, straight forward logic paths, short clarifying docs,...).

💚 Testing

I have manually tested my changes. (In Expensify App)

You can run FabricExample app in order to see that it also works correctly on new arch

Documentation

Select one:

I have added relevant documentation for my changes.
This PR does not result in any developer-facing changes. (theoretically since it should be transparent for the users)

WoLewicki · 2023-09-20T16:09:08Z

@dtroupe-plaid for some reason the newest commits were not applied to the previous PR so here is another one. Could you test if it works correctly?

WoLewicki · 2024-01-23T15:35:22Z

@dtroupe-plaid is there a chance that it can be reviewed and merged somehow?

WoLewicki · 2024-01-23T15:45:39Z

I temporarily added dist folder to be able to use it in the project, but I'll delete it when it is time to merge it 🚀

dtroupe-plaid · 2024-02-01T19:12:48Z

@dtroupe-plaid is there a chance that it can be reviewed and merged somehow?

Yes. Sorry for the delay I was OOO and prior to that we had a packed roadmap. I will have time to look at this tomorrow in the hopes we can merge it next week 🤞 can you resolve any conflicts and merge master (this change will only be supported in V11.x) 🙏

dtroupe-plaid

@WoLewicki I reviewed the PR and don't have many comments on the code. However, I do have some concerns about the example applications. Both examples work on iOS, but I was unable to get either example to run successfully on Android.

For the existing example app npm run android fails with a compiler error.
For the Fabric example the app appears to crash or freeze immediately upon opening. (see attached screen recording).
I tested android using a Pixel 7 API 34

Once the issues with the example applications are resolved I can approve this 🙏

Additionally, the fabric example will need a simple README similar to the existing example (not a blocker on this PR).

Let me know if you have any questions or concerns.

Screen.Recording.2024-02-02.at.11.53.54.mov

dtroupe-plaid · 2024-02-02T17:21:55Z

    private const val LINK_TOKEN_PREFIX = "link"

-    const val TAG = "PlaidAndroid"
+    const val NAME = "PlaidAndroid"


dtroupe-plaid · 2024-02-02T17:26:46Z

+  override fun continueFromRedirectUriString(redirectUriString: String?) {
+    Log.e("PlaidModule", "continueFromRedirectUriString is not available on Android")
+  }
+
+  override fun create(token: String, noLoadingState: Boolean) {
+    Log.e("PlaidModule", "create is not available on Android")
+  }
+
+  override fun open(fullScreen: Boolean, onSuccess: Callback?, onExit: Callback?) {
+    Log.e("PlaidModule", "open is not available on Android")
+  }
+
+  override fun dismiss() {
+    Log.e("PlaidModule", "dismiss is not available on Android")
+  }


Guessing we need to have these methods to support iOS?

We have to implement the methods of the interface

dtroupe-plaid · 2024-02-02T17:29:07Z

+- (void)continueFromRedirectUriString:(NSString *)redirectUriString { 
+    RCTLogError(@"continueFromRedirectUriString is not supported on iOS");
+}


If this isn't supported on iOS or Android should be remove this method?

Yeah, I left it since it was present in JS but I can remove it everywhere 🚀

dtroupe-plaid · 2024-02-02T17:31:49Z

    "typescript": "^4.9.5"
+  },
+  "dependencies": {
+    "react-native-plaid-link-sdk": "^10.4.0"


Suggested change

"react-native-plaid-link-sdk": "^10.4.0"

"react-native-plaid-link-sdk": "^11.5.0"

Adapt changes to master

WoLewicki · 2024-03-04T17:38:37Z

@dtroupe-plaid could you check again if everything builds correctly and there are no more crashes? And if so, could we get it merged and released? 🚀

davecusatis · 2024-04-05T13:12:52Z

@dtroupe-plaid @WoLewicki I would also love to see this get merged :)

WoLewicki · 2024-04-05T13:23:11Z

I am open to merging the newest master branch to resolve conflicts.

dtroupe-plaid · 2024-04-05T21:54:28Z

I should have time to review this next week. If everything works we can merge it 😄

WoLewicki · 2024-04-10T08:28:34Z

@dtroupe-plaid I resolved conflicts and migrated the new view to new arch also. Could you review it and test that the view behaves correctly so we can merge it?

dtroupe-plaid

The current Android example still does not work. I can't approve this until the example applications work.

I apologize for the delay - Please test the Android example app and ensure it works before requesting another review 🙏

dtroupe-plaid · 2024-05-08T22:20:31Z

+import com.facebook.react.viewmanagers.PLKEmbeddedViewManagerDelegate
+import com.facebook.react.viewmanagers.PLKEmbeddedViewManagerInterface


The changes in this file prevent the current example from compiling for Android.

file:///Users/dtroupe/plaid/client/link/react-native/react-native-plaid-link-sdk/example/node_modules/react-native-plaid-link-sdk/android/src/main/java/com/plaid/PLKEmbeddedViewManager.kt:8:40 Unresolved reference: PLKEmbeddedViewManagerDelegate

Failed to install the app. Command failed with exit code 1: ./gradlew app:installDebug -PreactNativeDevServerPort=8081 e: file:///Users/dtroupe/plaid/client/link/react-native/react-native-plaid-link-sdk/example/node_modules/react-native-plaid-link-sdk/android/src/main/java/com/plaid/PLKEmbeddedViewManager.kt:11:3 Unresolved reference: PLKEmbeddedViewManagerInterface e: file:///Users/dtroupe/plaid/client/link/react-native/react-native-plaid-link-sdk/example/node_modules/react-native-plaid-link-sdk/android/src/main/java/com/plaid/PLKEmbeddedViewManager.kt:15:16 Unresolved reference: PLKEmbeddedViewManagerDelegate e: file:///Users/dtroupe/plaid/client/link/react-native/react-native-plaid-link-sdk/example/node_modules/react-native-plaid-link-sdk/android/src/main/java/com/plaid/PLKEmbeddedViewManager.kt:27:3 'setToken' overrides nothing e: file:///Users/dtroupe/plaid/client/link/react-native/react-native-plaid-link-sdk/example/node_modules/react-native-plaid-link-sdk/android/src/main/java/com/plaid/PLKEmbeddedViewManager.kt:31:3 'setIOSPresentationStyle' overrides nothing FAILURE: Build failed with an exception

should not be approved

dtroupe-plaid

The current Android example app doesn't compile I can't approve this until the example application works.

I apologize for the delay - I would like to get this merged to help, but I don't get a lot of time allocated to this. Please test the current Android example locally to resolve these issues before requesting another review 🙏

You can test local changes with yalc like this:

tsc && yalc publish && cd example && yalc add react-native-plaid-link-sdk && npm install && npm run android

WoLewicki · 2024-05-09T13:53:56Z

@dtroupe-plaid could you check again? I added missing files.

dtroupe-plaid

Retested everything looks 👌

WoLewicki added 18 commits February 24, 2023 14:47

feat: add FabricExample app

6a90954

feat: add code from example

6bd0dc6

feat: update metro config

4c7208e

feat: update metro config

290e029

chore: more changes

58a7f06

feat: make ios compile

f81231b

feat: add JS spec and codegenConfig

8c44c2c

feat: change podspec nad iOS impl

ffe6630

feat: change build.gradle and android code

3432756

feat: run FabricExample on new arch

6c8728c

fix: remove unnecessary linking on Android

83a9cfd

fix: add fabric folder to files

8bea9d5

fix: types and add listener methods to spec

5198158

fix: add other files so library can be installed from branch

a51ac0a

chore: merge current main and add some changes

aaac3df

chore: bump FabricExample to 0.72

eeb260c

chore: merge current develop

3b34a1d

feat: bump ios and tvos required versions

56b9520

dtroupe-plaid changed the base branch from develop to master November 9, 2023 18:14

WoLewicki added 3 commits January 12, 2024 14:52

chore: merge current master

77f77e5

chore: bump FabricExample to 0.73.2

8990578

chore: merge current master again

6f8f5d8

fix: temporarly add dist to have types

94f894e

dtroupe-plaid reviewed Feb 2, 2024

View reviewed changes

roryabraham mentioned this pull request Feb 7, 2024

Enable Fabric and TurboModules on iOS & Android Expensify/App#13767

Merged

50 tasks

Merge branch 'master' into @wolewicki/add-fabric

4e34c95

j-piasecki and others added 5 commits February 8, 2024 17:24

Adapt to master

4f635b3

Add dist

d0084c6

Merge pull request #1 from j-piasecki/@wolewicki/add-fabric

d1b092c

Adapt changes to master

Merge branch 'master' into @wolewicki/add-fabric

3bad8b4

fix: remove circular dependency

633ed70

WoLewicki added 8 commits April 9, 2024 12:05

chore: merge current main

35fa44b

chore: bump to newest RN rc

67cb534

apply newest changes to modules

3f77fe1

chore: some ios configuration changes

18bbeda

chore: remove dist

3459457

chore: proper package-lock.json

df3c885

fix: make ts happy

a6aebe0

feat: migrate view to fabric

c68899e

dtroupe-plaid previously approved these changes May 8, 2024

View reviewed changes

dtroupe-plaid reviewed May 8, 2024

View reviewed changes

chore: merge current main and add missing files

9507f4e

dtroupe-plaid approved these changes May 9, 2024

View reviewed changes

dtroupe-plaid merged commit e521cfe into plaid:master May 9, 2024

	"react-native-plaid-link-sdk": "^10.4.0"
	"react-native-plaid-link-sdk": "^11.5.0"

		import com.facebook.react.viewmanagers.PLKEmbeddedViewManagerDelegate
		import com.facebook.react.viewmanagers.PLKEmbeddedViewManagerInterface

Conversation

WoLewicki commented Sep 20, 2023

Summary

Motivation

📝 Checklist

💚 Testing

Documentation

Uh oh!

WoLewicki commented Sep 20, 2023

Uh oh!

WoLewicki commented Jan 23, 2024

Uh oh!

WoLewicki commented Jan 23, 2024

Uh oh!

dtroupe-plaid commented Feb 1, 2024

Uh oh!

dtroupe-plaid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

WoLewicki Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

WoLewicki Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

WoLewicki commented Mar 4, 2024

Uh oh!

davecusatis commented Apr 5, 2024

Uh oh!

WoLewicki commented Apr 5, 2024

Uh oh!

dtroupe-plaid commented Apr 5, 2024

Uh oh!

WoLewicki commented Apr 10, 2024

Uh oh!

dtroupe-plaid left a comment

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid May 8, 2024

Choose a reason for hiding this comment

Uh oh!

dtroupe-plaid left a comment

Choose a reason for hiding this comment

Uh oh!

WoLewicki commented May 9, 2024

Uh oh!

dtroupe-plaid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dtroupe-plaid left a comment •

edited

Loading