-
Notifications
You must be signed in to change notification settings - Fork 6
iOS and Android native modules have different method signatures #25
Comments
@jakubkoci have you by any chance changed your mind about this? :) I think it would be easier to maintain if we don't include the To me that is in line with how React Native modules work. You implement the same API in both iOS and Android and expose that to JS so you don't have to worry about it. Just wanted to bring it up one more time before closing this issue. Otherwise I'll close it and rest my case forever :) |
You're right that we could get rid of Maybe the main reason I feel it's easier for me to do it in that way was that I needed to investigate signatures and data types conversions in Obj-C and Swift. But now when we have that knowledge. I would keep the |
Just to add my 2 cents--I agree that removing |
For non-implemented methods, I think it would be better to create a placeHolder method on the native side with a "NOT IMPLEMENTED" warning log or even throw an exception. I think the developer must be warned that that method is not implemented and might make the code fail. But up to you, I don't have an strong opinion. |
I'm closing the issue here because this repository is being archived. Issues will be managed in the new repository indy-sdk-react-native. The discussion can continue in hyperledger/indy-sdk-react-native#5 |
See the code samples below. for example
proverCreateCredentialReq
for iOS and Android sides expect different order of parameters. I guess because they match the native indy library for iOS/Android.However I think we should abstract that away on the native side and make the call from the JS side the same for both.
rn-indy-sdk/android/src/main/java/com/reactlibrary/IndySdkModule.java
Line 507 in 43aff51
rn-indy-sdk/ios/IndySdk.swift
Lines 302 to 304 in 43aff51
proverCreateCredentialReq
proverCreateCredentialReq
proverCreateCredentialReq
The text was updated successfully, but these errors were encountered: