Fixes undefined config variables in standalone builds leading to crashes #7
Conversation
WalkthroughThe changes in this pull request primarily involve the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant KindeAuthProvider
participant Environment
Developer->>KindeAuthProvider: Pass config { domain, clientId, scopes }
KindeAuthProvider->>KindeAuthProvider: Validate config values
KindeAuthProvider->>KindeAuthProvider: Initialize authentication
KindeAuthProvider->>Environment: Use domain and clientId for auth
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
readme.md (2)
26-31: LGTM! The new configuration approach is well documented.The inline configuration with clear comments about required and optional props provides a better developer experience.
Consider adding a brief explanation about why this approach is preferred over environment variables, particularly for standalone builds. This would help developers understand the rationale behind the change.
<KindeAuthProvider config={{ domain: "your-app.kinde.com", // Required clientId: "your-client-id", // Required // Optional (default: "openid profile email offline") scopes: "openid profile email offline", + }}> + {/* + * Note: Direct configuration is recommended over environment variables + * to ensure proper functionality in standalone builds (e.g., TestFlight, simulators) + */} <!-- Your application code -->
Line range hint
11-13: Remove or update the "Environment variables" section.The "Environment variables" section is now empty and only contains information about the redirect URL. Since the SDK no longer uses environment variables for configuration, this section should either be removed or updated to focus solely on the redirect URL functionality.
Consider either:
- Removing the section entirely, or
- Renaming it to "Redirect URL Configuration" and moving the redirect URL information there
lib/KindeAuthProvider.tsx (2)
47-54: Enhance error messages with setup instructions.While the validation is correct, the error messages could be more helpful by including setup instructions.
Consider updating the error messages:
- throw new Error("KindeAuthProvider config.domain prop is undefined"); + throw new Error( + "KindeAuthProvider config.domain prop is undefined. " + + "Please provide a valid domain in the KindeAuthProvider config prop. " + + "Example: <KindeAuthProvider config={{ domain: 'https://your-domain.kinde.com' }}>" + ); - throw new Error("KindeAuthProvider config.clientId prop is undefined"); + throw new Error( + "KindeAuthProvider config.clientId prop is undefined. " + + "Please provide a valid clientId in the KindeAuthProvider config prop. " + + "Example: <KindeAuthProvider config={{ clientId: 'your_client_id' }}>" + );
55-55: Optimize scopes initialization.The current implementation performs the split(" ") operation twice when config.scopes is undefined.
Consider this more efficient approach:
- const scopes = config.scopes?.split(" ") || DEFAULT_TOKEN_SCOPES.split(" "); + const scopesString = config.scopes || DEFAULT_TOKEN_SCOPES; + const scopes = scopesString.split(" ");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/KindeAuthProvider.tsx(1 hunks)readme.md(1 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/KindeAuthProvider.tsx (1)
Learnt from: didley
PR: kinde-oss/expo#5
File: lib/KindeAuthProvider.tsx:38-45
Timestamp: 2024-11-08T06:21:44.638Z
Learning: In `lib/KindeAuthProvider.tsx`, for the `KindeAuthProvider` component, the `config.domain` and `config.clientId` properties should be typed as `string | undefined` because users may pass `process.env` values, which can be `undefined`.
🔇 Additional comments (2)
lib/KindeAuthProvider.tsx (2)
38-45: LGTM! Well-structured config prop type definition.
The type definition correctly implements string | undefined for domain and clientId, which is appropriate for handling potential process.env values. The optional scopes field provides good flexibility.
38-55: Verify removal of process.env usage.
The implementation successfully moves configuration to props. Let's verify that all process.env usages have been removed.
✅ Verification successful
All process.env references successfully removed
The verification confirms that all process.env references related to Kinde configuration have been successfully removed from the codebase. The only remaining references are the error messages for validation of the new config props, which is the expected behavior after the migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining process.env usage related to Kinde configuration
# Search for process.env usage
echo "Checking for process.env usage..."
rg -i "process\.env\.(EXPO_PUBLIC_KINDE|KINDE)"
# Search for environment variable documentation
echo "Checking for environment variable documentation..."
rg -i "(EXPO_PUBLIC_KINDE|KINDE)_(DOMAIN|CLIENT_ID|SCOPES)"
Length of output: 299
Script:
#!/bin/bash
# Let's try a broader search to catch any remaining process.env usage
# and environment variables related to authentication configuration
# Search for process.env usage in the codebase
echo "Checking for any process.env usage..."
rg "process\.env" -A 2 -B 2
# Search for environment-related terms in documentation files
echo "Checking documentation files..."
fd -e md -e mdx -x cat {} \; | rg -i "(environment|env|KINDE|configuration)"
# Search for potential configuration patterns
echo "Checking for configuration patterns..."
rg "(KINDE|CLIENT_ID|AUTH.*DOMAIN)" -A 2 -B 2
Length of output: 1934
Script:
#!/bin/bash
# Let's do a more focused search in the lib directory and test files
# to ensure complete removal of environment variables
# Check the lib directory specifically
echo "Checking lib directory..."
fd . lib -t f -x cat {} \; | rg -i "(process\.env|KINDE.*CLIENT|KINDE.*DOMAIN)"
# Check test files for any remaining env references
echo "Checking test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts -x cat {} \; | rg -i "(process\.env|KINDE.*CLIENT|KINDE.*DOMAIN)"
# Check for any environment setup files
echo "Checking environment setup files..."
fd -e env -e env.example -e env.local -x cat {} \;
Length of output: 607
Explain your changes
This PR passes configuration variables via a
KindeAuthProviderconfig prop rather than accessing them internally viaprocess.env.We were having issues with this lib working fine locally but crashing within a standalone build, such as TestFlight or a simulator build, when login or register is called.
It appears to be due to how expo handles
EXPO_PUBLIC_variables. As noted in the expo docs,EXPO_PUBLIC_variables are replaced in your code but not withinnode_modules. Thoughnode_modulesEXPO_PUBLIC_variables appear to load when running your project locally.I've deployed this to a private package for testing with an iOS simulator build and it has fixed our issue.
May fix issue #4
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.