search

AbsaOSS / rn-indy-sdk

This code was starting point of https://github.com/hyperledger/indy-sdk-react-native where the work continues.
Apache License 2.0
11 stars 6 forks source link

iOS and Android native modules have different method signatures #25

Closed TimoGlastra closed 3 years ago

TimoGlastra commented 3 years ago

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.

https://github.com/AbsaOSS/rn-indy-sdk/blob/43aff51c158cb871c12ce4247aa7df74690e51da/android/src/main/java/com/reactlibrary/IndySdkModule.java#L507

https://github.com/AbsaOSS/rn-indy-sdk/blob/43aff51c158cb871c12ce4247aa7df74690e51da/ios/IndySdk.swift#L302-L304

TimoGlastra commented 3 years ago

@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 if / else statements on the JS side and instead handle it on the native side.

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 :)

jakubkoci commented 3 years ago

You're right that we could get rid of if statements and that's generally is a good thing :)

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 if statements when a method is not implemented for a particular platform. Sounds good?

JamesKEbert commented 3 years ago

Just to add my 2 cents--I agree that removing if statements at the JS level seems like a positive thing. But definitely if statements for platform-specific methods that have not been implemented.

icc-romeu commented 3 years ago

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.

jakubkoci commented 3 years ago

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