-
Notifications
You must be signed in to change notification settings - Fork 1
chore: update to latest launchdarkly api #180
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
chore: update to latest launchdarkly api #180
Conversation
suthar26
commented
Dec 17, 2025
- add proper support for the newest version of the API
| const encodedProjectKey = encodeURIComponent(projectKey) | ||
| const response = await fetch( | ||
| `${LD_BASE_URL}/flags/${encodedProjectKey}?summary=0`, | ||
| `${LD_BASE_URL}/projects/${encodedProjectKey}/environments`, |
Check warning
Code scanning / CodeQL
File data in outbound network request Medium
file data
| url += `&${envParams}` | ||
| } | ||
|
|
||
| const response = await fetch(url, { |
Check warning
Code scanning / CodeQL
File data in outbound network request Medium
file data
| + ' Please use API version 20220603 when creating the API access token.' | ||
| `Feature '${feature.key}' is missing environments. ` + | ||
| 'This may indicate that the feature flags were not fetched with environment data. ' + | ||
| 'Please use API version 20240415 when creating the API access token.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to not hardcode the version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its better to hardcode since that is expected, if we dont it uses the version that was used when creating the token, this way we know the expected outcome and readme is clear to get the latest version api token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the LaunchDarkly API integration from version 20220603 to 20240415 to support proper environment data fetching for feature flags. The update introduces a caching mechanism for environments and modifies how feature flags are retrieved to include environment-specific query parameters.
- Updates LaunchDarkly API version constant and related documentation
- Implements environment caching in the API wrapper to optimize feature flag requests
- Updates error messages to reflect the new API version requirement
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/api/LDApiWrapper.ts | Updates API version to 20240415, adds environment caching mechanism, introduces getEnvironments method, and modifies getFeatureFlagsForProject to use cached environment keys in query parameters |
| src/resources/features/LDFeatureImporter.ts | Updates error message to reference API version 20240415 and provides clearer context about missing environment data |
| README.md | Updates documentation to reflect API version 20240415, adds formatting improvements including blank lines and JSON indentation consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/api/LDApiWrapper.ts
Outdated
| ) | ||
| await this.handleErrors(response) | ||
| const environments = await response.json() | ||
| this.cachedEnvironments = environments.items.map((env: { key: string }) => env.key) ?? [] |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nullish coalescing operator (??) will never be used here because the map operation always returns an array (never undefined or null). If environments.items is undefined or null, the code will throw an error before reaching the ?? operator. Consider adding proper error handling or using optional chaining on the map operation.
| this.cachedEnvironments = environments.items.map((env: { key: string }) => env.key) ?? [] | |
| this.cachedEnvironments = environments.items?.map((env: { key: string }) => env.key) ?? [] |
| private async getEnvironments(projectKey: string) { | ||
| const headers = await this.getHeaders() | ||
| const encodedProjectKey = encodeURIComponent(projectKey) | ||
| const response = await fetch( | ||
| `${LD_BASE_URL}/flags/${encodedProjectKey}?summary=0`, | ||
| `${LD_BASE_URL}/projects/${encodedProjectKey}/environments`, | ||
| { | ||
| method: 'GET', | ||
| headers, | ||
| } | ||
| ) | ||
| await this.handleErrors(response) | ||
| const environments = await response.json() | ||
| this.cachedEnvironments = environments.items.map((env: { key: string }) => env.key) ?? [] | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getEnvironments method does not return a value but it's modifying instance state that is checked later. If environments.items is null or undefined, this will throw an error. Consider adding error handling or making the method return a value that can be checked.
| `Feature '${feature.key}' is missing environments. ` + | ||
| 'This may indicate that the feature flags were not fetched with environment data. ' + | ||
| 'Please use API version 20240415 when creating the API access token.' |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trailing space at the end of the string concatenation. This should be removed to maintain clean error messages.
| `Feature '${feature.key}' is missing environments. ` + | |
| 'This may indicate that the feature flags were not fetched with environment data. ' + | |
| 'Please use API version 20240415 when creating the API access token.' | |
| `Feature '${feature.key}' is missing environments. This may indicate that the feature flags were not fetched with environment data. Please use API version 20240415 when creating the API access token.` |
src/api/LDApiWrapper.ts
Outdated
| .map((key) => `env=${encodeURIComponent(key)}`) | ||
| .join('&') | ||
| url += `&${envParams}` | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trailing space at the end of line 93. This should be removed to maintain clean code formatting.
| } | |
| } |
src/api/LDApiWrapper.ts
Outdated
| this.apiToken = apiToken | ||
| } | ||
| apiToken: string | ||
| private cachedEnvironments: string[] |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cachedEnvironments property is declared but never initialized. If getProject is never called or if it's called but project.environments?.items is falsy, cachedEnvironments will remain undefined. The check on line 83 (!this.cachedEnvironments) will then trigger a call to getEnvironments, but if that also fails to set cachedEnvironments properly (e.g., if environments.items is null), the subsequent code will still reference an undefined value. Consider initializing this property to an empty array in the constructor.
src/api/LDApiWrapper.ts
Outdated
| private async getEnvironments(projectKey: string) { | ||
| const headers = await this.getHeaders() | ||
| const encodedProjectKey = encodeURIComponent(projectKey) | ||
| const response = await fetch( | ||
| `${LD_BASE_URL}/flags/${encodedProjectKey}?summary=0`, | ||
| `${LD_BASE_URL}/projects/${encodedProjectKey}/environments`, | ||
| { | ||
| method: 'GET', | ||
| headers, | ||
| } | ||
| ) | ||
| await this.handleErrors(response) | ||
| const environments = await response.json() | ||
| this.cachedEnvironments = environments.items.map((env: { key: string }) => env.key) ?? [] | ||
| } | ||
|
|
||
| async getFeatureFlagsForProject(projectKey: string) { | ||
| const headers = await this.getHeaders() | ||
| const encodedProjectKey = encodeURIComponent(projectKey) | ||
| if (!this.cachedEnvironments) { | ||
| await this.getEnvironments(projectKey) | ||
| } | ||
|
|
||
| let url = `${LD_BASE_URL}/flags/${encodedProjectKey}?summary=0` | ||
| if (this.cachedEnvironments && this.cachedEnvironments.length > 0) { | ||
| const envParams = this.cachedEnvironments | ||
| .map((key) => `env=${encodeURIComponent(key)}`) | ||
| .join('&') | ||
| url += `&${envParams}` | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new environment caching mechanism and getEnvironments method lack test coverage. Consider adding tests to verify: 1) environments are properly cached from getProject calls, 2) the fallback to getEnvironments works correctly when cachedEnvironments is undefined, 3) the URL construction with environment parameters works as expected, and 4) edge cases like empty or missing environments.items are handled correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/configs.ts
Outdated
| * Validates a key to prevent injection attacks in URL construction | ||
| * Keys should only contain lowercase alphanumeric characters, hyphens, underscores, and periods | ||
| * Keys should only contain alphanumeric characters (uppercase or lowercase), hyphens, underscores, and periods | ||
| * only the source Project Key can contain capital letters |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment capitalization inconsistency. The comment on line 39 starts with a lowercase letter "only" while other comments in the block start with capital letters. This should be capitalized for consistency: "Only the source Project Key can contain capital letters".
| * only the source Project Key can contain capital letters | |
| * Only the source Project Key can contain capital letters |
src/api/LDApiWrapper.ts
Outdated
|
|
||
| // Cache environment keys for use in feature flag requests | ||
| if (project.environments?.items) { | ||
| this.cachedEnvironments[projectKey] = project.environments?.items?.map( |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant optional chaining operator. The code already checks project.environments?.items in the if condition on line 39, so the second ?. on project.environments?.items?.map is unnecessary. The items property is already guaranteed to exist at this point.
| this.cachedEnvironments[projectKey] = project.environments?.items?.map( | |
| this.cachedEnvironments[projectKey] = project.environments?.items.map( |
|
|
||
| async getFeatureFlagsForProject(projectKey: string) { | ||
| private async getEnvironments(projectKey: string) { |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing return type annotation for the async getEnvironments method. This private method should explicitly declare that it returns Promise<void> for better type safety and code clarity.
| private async getEnvironments(projectKey: string) { | |
| private async getEnvironments(projectKey: string): Promise<void> { |
| + ' Please use API version 20220603 when creating the API access token.' | ||
| `Feature '${feature.key}' is missing environments. ` + | ||
| 'This may indicate that the feature flags were not fetched with environment data. ' + | ||
| 'Please use API version 20240415 when creating the API access token.' |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace after the period on line 111. This should be removed to maintain code cleanliness and consistency.
| 'Please use API version 20240415 when creating the API access token.' | |
| 'Please use API version 20240415 when creating the API access token.' |
src/configs.test.ts
Outdated
| expect(() => validateKey('.', 'testKey')).not.toThrow() | ||
| }) | ||
|
|
||
| test('should accept keys with all valid character types', () => { |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading test name and behavior. The test is named "should accept keys with all valid character types" but the first assertion expects the key 'aA1-_.' to throw an error when allowCapitalLetters is false (default). This test should either be moved to the "Invalid Keys" section or renamed to clarify it's testing the rejection of uppercase letters when not allowed.
| test('should accept keys with all valid character types', () => { | |
| test('should enforce capitalization rules for keys with all valid character types', () => { |
src/api/LDApiWrapper.ts
Outdated
| .map((key) => `env=${encodeURIComponent(key)}`) | ||
| .join('&') | ||
| url += `&${envParams}` | ||
| } |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace after the closing brace on line 95. This should be removed to maintain code cleanliness.
| } | |
| } |