BA-Add-mobile-graphql-environment #164
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/graphql/config/environment.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/graphql/.eslintrc.js
WalkthroughThis pull request involves version updates across multiple packages in the Changes
Sequence DiagramsequenceDiagram
participant App
participant GraphQLConfig
participant Environment
App->>GraphQLConfig: Request GraphQL Endpoint
GraphQLConfig->>Environment: Check NEXT_PUBLIC Endpoints
alt NEXT_PUBLIC Endpoints Not Found
GraphQLConfig->>Environment: Check EXPO_PUBLIC Endpoints
end
Environment-->>GraphQLConfig: Return Appropriate Endpoint
GraphQLConfig-->>App: Configure GraphQL Connection
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
🧹 Nitpick comments (2)
packages/graphql/CHANGELOG.md (1)
3-8: Enhance changelog entry with implementation detailsWhile the changelog entry correctly indicates the addition of Expo/Mobile support, it would be more helpful to specify the actual changes made (i.e., the addition of fallback environment variables for relay endpoints).
Consider expanding the entry like this:
- Add graphql support to Expo/Mobile environment + Add graphql support to Expo/Mobile environment by introducing EXPO_PUBLIC_RELAY_ENDPOINT and EXPO_PUBLIC_WS_RELAY_ENDPOINT as fallback environment variablespackages/graphql/config/environment.ts (1)
Line range hint
1-1: Add documentation for environment setupThe file lacks documentation about the required environment variables for both Next.js and Expo environments.
Consider adding a comment block at the top of the file:
/** * Environment Configuration * * Required environment variables: * - Next.js environment: * - NEXT_PUBLIC_RELAY_ENDPOINT: GraphQL HTTP endpoint * - NEXT_PUBLIC_WS_RELAY_ENDPOINT: GraphQL WebSocket endpoint * * - Expo environment: * - EXPO_PUBLIC_RELAY_ENDPOINT: GraphQL HTTP endpoint * - EXPO_PUBLIC_WS_RELAY_ENDPOINT: GraphQL WebSocket endpoint */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/graphql/CHANGELOG.md(1 hunks)packages/graphql/config/environment.ts(2 hunks)packages/graphql/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/graphql/package.json
- packages/wagtail/package.json
- packages/components/package.json
- packages/wagtail/CHANGELOG.md
- packages/components/CHANGELOG.md
🔇 Additional comments (1)
packages/graphql/config/environment.ts (1)
68-69: Verify environment variable usage across the codebase
Let's ensure that the new Expo environment variables are properly documented and used consistently across the codebase.
Also applies to: 93-94
✅ Verification successful
Based on the provided context, I can see that the environment variables NEXT_PUBLIC_RELAY_ENDPOINT, EXPO_PUBLIC_RELAY_ENDPOINT, NEXT_PUBLIC_WS_RELAY_ENDPOINT, and EXPO_PUBLIC_WS_RELAY_ENDPOINT are used consistently across the codebase. The search results show that:
- The environment variables are properly used in the GraphQL configuration (
packages/graphql/config/environment.ts) - They are correctly set in webpack configurations for development environments
- They are used in Cypress configurations for testing
- The variables follow a consistent naming pattern (NEXT_PUBLIC_* for Next.js and EXPO_PUBLIC_* for Expo)
Since the environment variables are properly documented in the code and used consistently across the codebase, I can generate the final response:
Environment variables are properly configured and used consistently
The new Expo environment variables are correctly implemented alongside their Next.js counterparts, maintaining consistency across the codebase. The variables are properly used in:
- GraphQL configuration for both HTTP and WebSocket endpoints
- Development webpack configurations
- Testing configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for environment variable usage and documentation
# Check for environment variable usage
echo "Checking for environment variable usage..."
rg "EXPO_PUBLIC_RELAY_ENDPOINT|NEXT_PUBLIC_RELAY_ENDPOINT|EXPO_PUBLIC_WS_RELAY_ENDPOINT|NEXT_PUBLIC_WS_RELAY_ENDPOINT"
# Check for environment documentation in README or similar files
echo -e "\nChecking for environment documentation..."
fd -t f "README|\.env|\.env\.example" -x cat {}
Length of output: 47317
| baseUrl: (process.env.NEXT_PUBLIC_RELAY_ENDPOINT || | ||
| process.env.EXPO_PUBLIC_RELAY_ENDPOINT) as string, |
There was a problem hiding this comment.
Add validation for required environment variables
The current implementation silently casts potentially undefined environment variables to string. This could lead to runtime errors if neither NEXT_PUBLIC_RELAY_ENDPOINT nor EXPO_PUBLIC_RELAY_ENDPOINT is defined.
Consider adding validation:
- baseUrl: (process.env.NEXT_PUBLIC_RELAY_ENDPOINT ||
- process.env.EXPO_PUBLIC_RELAY_ENDPOINT) as string,
+ baseUrl: (() => {
+ const endpoint = process.env.NEXT_PUBLIC_RELAY_ENDPOINT || process.env.EXPO_PUBLIC_RELAY_ENDPOINT;
+ if (!endpoint) {
+ throw new Error('Missing required environment variable: NEXT_PUBLIC_RELAY_ENDPOINT or EXPO_PUBLIC_RELAY_ENDPOINT');
+ }
+ return endpoint;
+ })(),📝 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.
| baseUrl: (process.env.NEXT_PUBLIC_RELAY_ENDPOINT || | |
| process.env.EXPO_PUBLIC_RELAY_ENDPOINT) as string, | |
| baseUrl: (() => { | |
| const endpoint = process.env.NEXT_PUBLIC_RELAY_ENDPOINT || process.env.EXPO_PUBLIC_RELAY_ENDPOINT; | |
| if (!endpoint) { | |
| throw new Error('Missing required environment variable: NEXT_PUBLIC_RELAY_ENDPOINT or EXPO_PUBLIC_RELAY_ENDPOINT'); | |
| } | |
| return endpoint; | |
| })(), |
| url: (process.env.NEXT_PUBLIC_WS_RELAY_ENDPOINT || | ||
| process.env.EXPO_PUBLIC_WS_RELAY_ENDPOINT) as string, |
There was a problem hiding this comment.
Add validation for WebSocket endpoint
Similar to the HTTP endpoint, the WebSocket endpoint should validate that at least one environment variable is defined.
Consider adding validation:
- url: (process.env.NEXT_PUBLIC_WS_RELAY_ENDPOINT ||
- process.env.EXPO_PUBLIC_WS_RELAY_ENDPOINT) as string,
+ url: (() => {
+ const wsEndpoint = process.env.NEXT_PUBLIC_WS_RELAY_ENDPOINT || process.env.EXPO_PUBLIC_WS_RELAY_ENDPOINT;
+ if (!wsEndpoint) {
+ throw new Error('Missing required environment variable: NEXT_PUBLIC_WS_RELAY_ENDPOINT or EXPO_PUBLIC_WS_RELAY_ENDPOINT');
+ }
+ return wsEndpoint;
+ })(),📝 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.
| url: (process.env.NEXT_PUBLIC_WS_RELAY_ENDPOINT || | |
| process.env.EXPO_PUBLIC_WS_RELAY_ENDPOINT) as string, | |
| url: (() => { | |
| const wsEndpoint = process.env.NEXT_PUBLIC_WS_RELAY_ENDPOINT || process.env.EXPO_PUBLIC_WS_RELAY_ENDPOINT; | |
| if (!wsEndpoint) { | |
| throw new Error('Missing required environment variable: NEXT_PUBLIC_WS_RELAY_ENDPOINT or EXPO_PUBLIC_WS_RELAY_ENDPOINT'); | |
| } | |
| return wsEndpoint; | |
| })(), |
graphqlpackage update -v 1.1.14Summary by CodeRabbit
@baseapp-frontend/components,@baseapp-frontend/graphql, and@baseapp-frontend/wagtailpackages.