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

Expose initialiseWith* functions on react-native #83

Merged
merged 13 commits into from
Jan 27, 2025
Merged

Conversation

JNdhlovu
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/14700/expose-initialisewith-functions-on-react-native

Summary

So far, the react-native sdk doesn't expose the initializeWithApiKey and initializeWithConfig function in the SmileID object. The underlying ios and android implementation already have those functions exposed, we're just missing to expose them in the SmileID object.

Known Issues

  • I've not been able to test this on XCode 16

Test Instructions

  • Inititalize using the config.json which is the existing method and run a job
  • Initialize with api key and run a job
  • Initialize with the config.json object

Screenshot

N/A

@prfectionist
Copy link

prfectionist bot commented Jan 15, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

API Key Exposure:
The new initializeWithApiKey function accepts an API key parameter. Need to ensure this API key is properly secured and not logged or exposed in any way during initialization.

⚡ Recommended focus areas for review

Parameter Validation
The new initialization functions don't validate input parameters. Should check for null/undefined values and validate config object structure.

Documentation
The new initialization functions lack JSDoc documentation about parameters and usage, unlike the existing initialize function.

@prfectionist
Copy link

prfectionist bot commented Jan 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add input validation to prevent runtime errors from invalid parameters

Add parameter validation to ensure apiKey is not empty or undefined before passing
it to the native module.

src/index.tsx [72-83]

 initializeWithApiKey: (
   apiKey: string,
   config: Config,
   useSandBox: boolean,
   enableCrashReporting: boolean
-) =>
-  _SmileID.initializeWithApiKey(
+) => {
+  if (!apiKey?.trim()) {
+    throw new Error('API key cannot be empty');
+  }
+  return _SmileID.initializeWithApiKey(
     apiKey,
     config,
     useSandBox,
     enableCrashReporting
-  ),
+  );
+},
Suggestion importance[1-10]: 8

Why: Adding validation for the API key is crucial for preventing runtime errors and providing clear error messages, especially since this is an initialization method where the API key is a critical parameter.

8
Enhancement
Add default parameter values to improve API usability and consistency

Add default values for useSandBox and enableCrashReporting parameters to maintain
consistency with the existing initialize method and provide sensible defaults.

src/index.tsx [72-77]

 initializeWithApiKey: (
   apiKey: string,
   config: Config,
-  useSandBox: boolean,
-  enableCrashReporting: boolean
+  useSandBox: boolean = false,
+  enableCrashReporting: boolean = true
 )
Suggestion importance[1-10]: 7

Why: Adding default values for parameters would maintain consistency with the existing initialize method and improve developer experience by reducing required parameters. This aligns with the existing pattern shown in line 70.

7

@robin-ankele robin-ankele requested a review from a team January 15, 2025 15:25
@robin-ankele
Copy link
Contributor

@JNdhlovu, @jumaallan I've noticed that the following params in the config object differ between android and iOS:

  • iOS: prodLambdaUrl, testLambdaUrl
  • Android: prodBaseUrl, sandboxBaseUrl

Does is make sense to change the Mapper for android to grab the keys as defined in

prodLambdaUrl: string;
and map to the android definitions:
prodBaseUrl = getStringOrDefault("prodBaseUrl") ?: run {

sandboxBaseUrl = getStringOrDefault("sandboxBaseUrl") ?: run {

Copy link
Contributor

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

Tested on iOS and android side. Works fine. Left a comment on the Config object.

@tobitech
Copy link

I was able to test this on iOS. Tested Selfie Capture, Biometric KYC and Document Verification.

Copy link
Contributor

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

As discussed in slack, should we add a section to the README to talk about the new initalisation params for partners to fill in?

example/src/HomeScreen.tsx Outdated Show resolved Hide resolved
example/src/HomeScreen.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

Works fine on android and iOS. I've tested the following configurations:

@JNdhlovu JNdhlovu merged commit 4d3c0cc into main Jan 27, 2025
6 checks passed
@JNdhlovu JNdhlovu deleted the bugfix/initialize branch January 27, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants