Fix issue regarding skipping env variables, suppressing non-dev error messages, parsing URL-formatted env values and handling empty server command inputs. #75
Conversation
CL-2062 | +Harshi | Fix skip environment variables option with --vari…
fix: suppress error message in non-development environments
Add AGENTS.md file
fix: environment variable parsing for URL formatted values
fix: allow --variable-type flag to support multiple selections
fix: handle empty server command input properly
Fix issue regarding skipping env variables, suppressing non-dev error messages, parsing URL-formatted env values and handling empty server command inputs.
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple issues related to environment variable handling, Apollo Client error message suppression, server command input validation, and CLI flag configuration. The changes focus on improving user experience by suppressing noisy Apollo deprecation warnings in production, properly parsing environment variables with URL formats, and allowing multiple variable import options.
Key Changes:
- Implemented console patching to suppress Apollo-specific error/warning messages in non-development environments
- Enhanced environment variable parsing to support URL-formatted values (handling multiple colons)
- Modified
--variable-typeflag to accept multiple values for flexible environment setup
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/apollo-client.ts | Adds console patching logic and isNotDevelopment export for suppressing Apollo messages |
| test/unit/util/apollo-client.test.ts | New Mocha/Chai test suite validating console suppression behavior |
| src/util/logs-polling-utilities.ts | Adds withDeprecationsDisabled helper to wrap Apollo watchQuery calls |
| src/adapters/base-class.ts | Refactors env parsing to support URL values; converts single variableType to array |
| src/adapters/base-class.test.ts | Comprehensive tests for string-to-array conversion and multiple variable type handling |
| src/adapters/github.ts | Updates server command handling to allow empty input; uses new variableType logic |
| src/adapters/github.test.ts | New Jest tests for server command prompting and deployment flow scenarios |
| src/adapters/file-upload.ts | Mirrors github.ts changes for server command and variableType handling |
| src/adapters/file-upload.test.ts | New Jest tests matching github.test.ts coverage for FileUpload adapter |
| src/commands/launch/index.ts | Adds multiple: true to --variable-type flag and updates documentation |
| package.json, package-lock.json | Upgrades @apollo/client from ^3.11.8 to ^3.14.0 |
| AGENTS.md | New file documenting testing conventions for AI agents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @method withDeprecationsDisabled - Helper to disable Apollo Client deprecation warnings | ||
| * | ||
| * Wraps the provided function so that Apollo deprecation warnings are disabled | ||
| * only during its execution, and restored immediately after. | ||
| */ | ||
| private withDeprecationsDisabled<T>(fn: () => T): T { | ||
|
|
||
| if (!isNotDevelopment) { | ||
| return fn(); | ||
| } | ||
|
|
||
| let withDisabledDeprecations: any; | ||
| try { | ||
| withDisabledDeprecations = require('@apollo/client/utilities/deprecation').withDisabledDeprecations; | ||
| } catch { | ||
| return fn(); | ||
| } | ||
|
|
||
| const handler = withDisabledDeprecations(); | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| this.disposeDeprecationHandler(handler); | ||
| } | ||
| } |
There was a problem hiding this comment.
The deprecation suppression is enabled in non-development environments, but the withDisabledDeprecations import might fail if the Apollo Client version doesn't export it from @apollo/client/utilities/deprecation. While there is a try-catch that handles the require failure, the function documentation doesn't mention this limitation or the version dependency. Consider adding a comment explaining that this functionality requires Apollo Client 3.14.0 or later, or document the graceful degradation when the import fails.
| private withDeprecationsDisabled<T>(fn: () => T): T { | ||
|
|
||
| if (!isNotDevelopment) { | ||
| return fn(); | ||
| } | ||
|
|
||
| let withDisabledDeprecations: any; | ||
| try { | ||
| withDisabledDeprecations = require('@apollo/client/utilities/deprecation').withDisabledDeprecations; | ||
| } catch { | ||
| return fn(); | ||
| } | ||
|
|
||
| const handler = withDisabledDeprecations(); | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| this.disposeDeprecationHandler(handler); | ||
| } | ||
| } | ||
|
|
||
| private disposeDeprecationHandler(handler?: any): void { | ||
| if (!handler) return; | ||
| try { | ||
| const dispose = (handler as any)[(Symbol as any).dispose]; | ||
| if (typeof dispose === 'function') { | ||
| dispose.call(handler); | ||
| return; | ||
| } | ||
| } catch {} | ||
| try { | ||
| const asyncDispose = (handler as any)[(Symbol as any).asyncDispose]; | ||
| if (typeof asyncDispose === 'function') { | ||
| asyncDispose.call(handler); | ||
| return; | ||
| } | ||
| } catch {} | ||
| try { | ||
| const symbols = Object.getOwnPropertySymbols(handler); | ||
| for (const sym of symbols) { | ||
| const maybeDispose = (handler as any)[sym]; | ||
| if (typeof maybeDispose === 'function') { | ||
| maybeDispose.call(handler); | ||
| break; | ||
| } | ||
| } | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
The new withDeprecationsDisabled and disposeDeprecationHandler methods added to the LogPolling class lack test coverage. Consider adding unit tests to verify that these methods correctly wrap watchQuery calls and properly handle edge cases such as: when the deprecation utilities are unavailable (require fails), when the handler disposal methods fail, and when running in development vs non-development environments.
| environmentVariables: map(this.envVariables, ({ key, value }) => ({ key, value })), | ||
| buildCommand: buildCommand === undefined || buildCommand === null ? 'npm run build' : buildCommand, | ||
| serverCommand: serverCommand === undefined || serverCommand === null ? 'npm run start' : serverCommand, | ||
| ...(serverCommand && serverCommand.trim() !== '' ? { serverCommand } : {}), |
There was a problem hiding this comment.
The spread operator pattern used here conditionally includes serverCommand only when it's truthy and not empty after trimming. However, the serverCommand value coming from this.config might already be empty or undefined at this point. Consider ensuring that this.config.serverCommand is set only when a valid non-empty value is provided, or add validation earlier in the flow to avoid relying on runtime checks at the mutation level.
Update form-data to 4.0.4 and add dependency overrides
fix: Update form-data to 4.0.4 and add dependency overrides
fix: skip environment variables option with --variable-type flag
fix: suppress error message in non-development environments
fix: environment variable parsing for URL formatted values
fix: allow --variable-type flag to accept multiple values
fix: handle empty server command input properly