Fixes undefined config variables in standalone builds leading to crashes #5
Conversation
Passes config variables via KindeAuthProvider config prop rather than deriving from process.env
WalkthroughThe changes in this pull request introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant KindeAuthProvider
User->>App: Initialize App
App->>KindeAuthProvider: Pass config { domain, clientId, scopes }
KindeAuthProvider->>KindeAuthProvider: Validate config
KindeAuthProvider->>KindeAuthProvider: Initialize authentication
KindeAuthProvider->>User: Provide authentication methods
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
readme.md (1)
Line range hint
1-100: Add sections for troubleshooting and deployment considerations.Given that this PR addresses critical issues with standalone builds, the documentation should include additional sections to help users avoid common pitfalls.
Please add the following sections:
## Deployment Considerations ### Standalone Builds (TestFlight, Simulators) The configuration must be provided via the `config` prop as environment variables are not reliably accessible in standalone builds. This is particularly important for: - TestFlight builds - Simulator builds - Production deployments ## Troubleshooting ### Common Issues 1. **Undefined Configuration** - Symptom: App crashes on login/register in standalone builds - Solution: Ensure configuration is provided via the `config` prop, not environment variables 2. **Invalid Domain Format** - Symptom: Authentication fails with domain-related errors - Solution: Ensure domain is in the format "your-app.kinde.com" without any brackets or extra formatting ## Version Compatibility - Expo SDK: X.Y.Z or higher - React Native: X.Y.Z or higherlib/KindeAuthProvider.tsx (2)
47-54: Consider consolidating validation checks.While the validation logic is correct, it could be more maintainable. With the improved types from the previous suggestion, you could either:
- Remove the runtime checks entirely (they become redundant with proper typing), or
- Consolidate them into a single validation function
- const domain = config.domain; - if (domain === undefined) - throw new Error("KindeAuthProvider config.domain prop is undefined"); - - const clientId = config.clientId; - if (clientId === undefined) - throw new Error("KindeAuthProvider config.clientId prop is undefined"); + const { domain, clientId } = config; + const validateConfig = () => { + const missing = []; + if (!domain) missing.push('domain'); + if (!clientId) missing.push('clientId'); + if (missing.length > 0) { + throw new Error(`KindeAuthProvider missing required config: ${missing.join(', ')}`); + } + }; + validateConfig();
55-55: Document default scopes behavior.The fallback to
DEFAULT_TOKEN_SCOPESis implemented correctly, but it would be helpful to document this behavior for future maintainers.+ // If scopes are not provided, fall back to DEFAULT_TOKEN_SCOPES + // DEFAULT_TOKEN_SCOPES typically includes essential permissions needed for basic authentication const scopes = config.scopes?.split(" ") || DEFAULT_TOKEN_SCOPES.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 comments (1)
lib/KindeAuthProvider.tsx (1)
Line range hint 1-55: Verify complete removal of environment variable usage.
The changes look good and align with the PR objectives of fixing undefined config variables in standalone builds. Let's verify that all environment variable usage has been removed from the codebase.
✅ Verification successful
Environment variable usage has been completely removed ✅
The verification confirms that there are no remaining instances of environment variable usage (process.env) in the codebase, including EXPO_PUBLIC_ prefixed variables. This aligns perfectly with the PR's objective of fixing undefined config variables in standalone builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining environment variable usage
# that might affect standalone builds
echo "Checking for any remaining EXPO_PUBLIC_ environment variable usage..."
rg -l "process\.env\.EXPO_PUBLIC_" || echo "No EXPO_PUBLIC_ environment variables found."
echo "Checking for any direct environment variable access..."
rg -l "process\.env\." || echo "No direct environment variable access found."
Length of output: 527
| <KindeAuthProvider config={{ | ||
| domain: [yourapp.kinde.com], | ||
| clientId: "ApplicationClientId", | ||
| // Optional (default: "openid profile email offline") | ||
| scopes: "openid profile email offline", | ||
| }}> |
There was a problem hiding this comment.
Documentation needs improvements for clarity and completeness.
Several issues need to be addressed in the configuration example:
- The domain placeholder syntax is incorrect
- Missing clear indication of which config fields are required
- No migration guide for users moving from environment variables
Apply these changes to improve the documentation:
<KindeAuthProvider config={{
- domain: [yourapp.kinde.com],
+ domain: "your-app.kinde.com", // Required
- clientId: "ApplicationClientId",
+ clientId: "your-client-id", // Required
// Optional (default: "openid profile email offline")
scopes: "openid profile email offline",
}}>Also, please add a migration section above the configuration example:
## Migration from environment variables
As of version X.Y.Z, we've moved from environment variables to explicit configuration
to ensure reliability in standalone builds. If you're upgrading from a previous version,
replace your environment variables with the config prop as shown below.
Previous:
```env
EXPO_PUBLIC_KINDE_DOMAIN=your-app.kinde.com
EXPO_PUBLIC_KINDE_CLIENT_ID=your-client-id
EXPO_PUBLIC_KINDE_SCOPES=openid profile email offlineCurrent:
See the configuration example below.
<!-- This is an auto-generated comment by CodeRabbit -->
| config, | ||
| }: { | ||
| children: React.ReactNode; | ||
| config: { | ||
| domain: string | undefined; | ||
| clientId: string | undefined; | ||
| scopes?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen type safety for config prop.
The current type definition allows undefined values for required fields, which doesn't align with the runtime checks. Consider updating the type definition to enforce required values at compile-time:
config: {
- domain: string | undefined;
- clientId: string | undefined;
+ domain: string;
+ clientId: string;
scopes?: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config, | |
| }: { | |
| children: React.ReactNode; | |
| config: { | |
| domain: string | undefined; | |
| clientId: string | undefined; | |
| scopes?: string; | |
| }; | |
| config, | |
| }: { | |
| children: React.ReactNode; | |
| config: { | |
| domain: string; | |
| clientId: string; | |
| scopes?: string; | |
| }; |
There was a problem hiding this comment.
domain: string | undefined;
clientId: string | undefined;
I feel this typing is correct as they are being asserted internally now and many users will be passing process.env values which will always be string | undefined.
There was a problem hiding this comment.
@didley, thank you for the clarification. I understand that users may pass process.env values, which can be string | undefined, so keeping the typings as string | undefined is appropriate. I've noted this for future reviews.
✏️ Learnings added
Learnt from: didley
PR: kinde-oss/expo#5
File: lib/KindeAuthProvider.tsx:38-45
Timestamp: 2024-11-08T06:21:44.501Z
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`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
vite.config.ts (1)
11-11: Consider making minification configurable based on build environment.While disabling minification can help with debugging production issues, it's generally recommended to keep it enabled for production builds. Consider making this configurable through an environment variable.
- minify: false, + minify: process.env.NODE_ENV === 'development' ? false : true,lib/KindeAuthProvider.tsx (1)
41-58: LGTM! Consider enhancing error messages.The config prop implementation correctly handles required values with proper runtime validation. The typing as
string | undefinedaligns with the use case of passingprocess.envvalues.Consider enhancing the error messages to be more descriptive:
- throw new Error("KindeAuthProvider config.domain prop is undefined"); + throw new Error("KindeAuthProvider requires a valid domain. Please provide config.domain in your KindeAuthProvider configuration."); - throw new Error("KindeAuthProvider config.clientId prop is undefined"); + throw new Error("KindeAuthProvider requires a valid clientId. Please provide config.clientId in your KindeAuthProvider configuration.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
lib/KindeAuthProvider.tsx(1 hunks)vite.config.ts(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)
vite.config.ts (1)
11-11: Verify the impact on bundle size.
Disabling minification will increase the bundle size. Let's verify the impact:
lib/KindeAuthProvider.tsx (1)
32-37: LGTM! Verify polyfill functionality in web builds.
The polyfill implementation for atob and btoa looks correct and is properly guarded. The comment clearly explains its purpose for web expo export script support.
Let's verify the polyfill implementation:
✅ Verification successful
Polyfill implementation is correctly isolated and dependencies are properly configured
The verification shows:
- The base64 polyfill is only implemented in
KindeAuthProvider.tsx - The required
base-64package is properly listed in dependencies - No conflicting implementations were found in the codebase
- Type definitions are correctly included via
@types/base-64
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other base64 encoding/decoding implementations or dependencies
# that might conflict with these polyfills.
# Test: Search for base64 related implementations
rg -l 'atob|btoa|base64' --type ts --type tsx
# Test: Check package.json for base64 related dependencies
fd package.json --exec jq -r '.dependencies + .devDependencies | keys[]' {} | rg -i 'base64|encoding'
Length of output: 181
Script:
#!/bin/bash
# Let's try a different approach to search for base64 related implementations
# and dependencies
# Search for TypeScript/JavaScript files containing base64 related code
rg -l "atob|btoa|base64" --type ts --type js
# Check package.json dependencies more directly
if [ -f "package.json" ]; then
cat package.json | jq -r '.dependencies,.devDependencies' 2>/dev/null
fi
# Look for imports of base-64 package
rg -l "from ['|\"]base-64['|\"]" --type ts --type js
Length of output: 866
|
Closing PR to replace with #7 without any of my own publishing code |
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.