Backend request improvements #14642
Backend request improvements #14642
Conversation
* Add the `fullResponse` flag as an option for `makeRequest` to return the raw HTTP response * Deprecate the `environment_name` field when creating a Connect token, in favour of the `environment` name in the client's constructor
jverce
added
enhancement
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@jverce is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/server/index.ts (2)
298-305: LGTM! Well-documented feature addition.The
fullResponseoption is well-documented, with clear explanation of its behavior and implications for error handling. Consider adding a code example in the documentation to demonstrate the difference in error handling betweenfullResponse: trueandfalse.Example addition to the documentation:
* code in the response object. Defaults to false. */ fullResponse?: boolean; + /** + * @example + * ```typescript + * // With fullResponse: false (default) + * try { + * const result = await client.makeRequest('/api/data'); + * // result contains parsed response body + * } catch (error) { + * // error thrown for non-200 responses + * } + * + * // With fullResponse: true + * const response = await client.makeRequest('/api/data', { fullResponse: true }); + * if (!response.ok) { + * // handle error case + * } + * const data = await response.json(); + * ``` + */
Line range hint
436-489: Improve type safety forfullResponseoption.While the implementation works, the current type casting approach might hide type errors. Consider using a generic type parameter with a conditional type to provide better type safety.
Here's a suggested improvement:
- public async makeRequest<T>( + public async makeRequest<T, R = T>( path: string, - opts: RequestOptions = {}, + opts: RequestOptions & { fullResponse?: false } | RequestOptions & { fullResponse: true }, - ): Promise<T> { + ): Promise<opts extends { fullResponse: true } ? Response : T> { const { params, headers: customHeaders, body, method = "GET", baseURL = this.baseApiUrl, fullResponse = false, ...fetchOpts } = opts; // ... rest of the implementation ... const response: Response = await fetch(url.toString(), requestOptions); if (fullResponse) { - return response as unknown as T; + return response as any; // Type assertion needed due to TypeScript limitations } // ... rest of the implementation ... }This change will:
- Provide proper type hints to TypeScript users
- Make it clear that
fullResponse: truereturns aResponseobject- Maintain type safety for the default case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/sdk/package.json(1 hunks)packages/sdk/src/server/index.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/package.json
🔇 Additional comments (1)
packages/sdk/src/server/index.ts (1)
90-92: LGTM! Clear deprecation notice.
The deprecation message clearly indicates that users should use the environment field in BackendClientOpts instead, and explicitly states that this field is ignored.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/CHANGELOG.md (2)
8-11: Consider adding a migration example.The deprecation notice is clear but would benefit from a brief example showing how to migrate from
environment_nameto usingenvironmentin the client constructor.Example addition:
is no longer used by the SDK nor the Connect API. The environment name is now exclusively determined by the `environment` field in the `BackendClientOpts` type, read during the client creation. + + Migration example: + ```typescript + // Before + const token = await client.createConnectToken({ environment_name: 'prod' }); + + // After + const client = new BackendClient({ environment: 'prod' }); + const token = await client.createConnectToken(); + ```
15-17: Consider adding a usage example and response type details.While the new feature is well documented, it would be more helpful with a usage example and specific details about the response type.
Example addition:
allows the user to get the full HTTP response object, including the headers and status code. + + Example: + ```typescript + // Before + const data = await client.makeRequest('/api/v1/resource'); + + // After + const response = await client.makeRequest('/api/v1/resource', { fullResponse: true }); + console.log(response.status); // 200 + console.log(response.headers); // Response headers + console.log(response.data); // Response body + ``` + + The full response object follows the standard HTTP response interface with: + - `status`: number + - `headers`: Record<string, string> + - `data`: any
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/CHANGELOG.md(1 hunks)
🔇 Additional comments (1)
packages/sdk/CHANGELOG.md (1)
4-5: LGTM! Version bump follows semver.
The version increment to 1.0.1 is appropriate for these backward-compatible changes.
…makerequest-method-from-sdk
jverce
deleted the
jay/dj-2654-return-full-fetch-response-in-makerequest-method-from-sdk
branch
Changelog
fullResponseflag as an option formakeRequestto return the raw HTTP responseenvironment_namefield when creating a Connect token, in favour of theenvironmentname in the client's constructorWHY
To provide customers with more flexibility when making requests against the Connect API.
Summary by CodeRabbit
New Features
fullResponseproperty in theRequestOptionsinterface, allowing users to receive complete HTTP response objects.environment_namefield inConnectTokenOpts, streamlining the configuration process for connect tokens.Bug Fixes
makeRequestmethod to support the newfullResponsefunctionality.Chores
@pipedream/sdkpackage to 1.0.2.