feat(core): if provided options.request use instead #3944
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 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: 2
🧹 Outside diff range and nitpick comments (2)
packages/@webex/http-core/src/request/index.js (1)
26-33: Update JSDoc to document the newoptions.requestparameter.The function's JSDoc should be updated to document the new capability of providing a custom request handler.
Add the following to the JSDoc:
/** * @param {Object} options + * @param {Function} [options.request] - Optional custom request handler function * @returns {Promise} */packages/@webex/http-core/test/unit/spec/request/index.js (1)
21-40: Consider enhancing test coverage and readability.While the test effectively validates the core functionality, consider these improvements:
- Add assertions for the interceptor chain's behavior
- Use more descriptive test names that follow the given-when-then pattern
- Extract test data into named constants for better maintainability
Example improvement:
- it('should modify options and call _request if no custom request function is provided', async () => { + it('given default configuration, when making a request, then should use internal request handler', async () => { + // Test setup + const TEST_URL = 'http://example.com'; + const EXPECTED_RESPONSE = 'response'; const options = { - url: 'http://example.com', + url: TEST_URL, headers: {}, interceptors: [], }; - requestStub.resolves('response'); + requestStub.resolves(EXPECTED_RESPONSE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/http-core/src/request/index.js(1 hunks)packages/@webex/http-core/test/unit/spec/index.js(0 hunks)packages/@webex/http-core/test/unit/spec/request/index.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/@webex/http-core/test/unit/spec/index.js
🔇 Additional comments (5)
packages/@webex/http-core/src/request/index.js (2)
26-36: Implementation looks clean and maintains compatibility.
The changes:
- Preserve the existing Promise chain structure
- Maintain backward compatibility
- Correctly handle response interceptors
- Keep the event emitter functionality intact
28-30: Verify custom request handler compatibility.
The custom request handler must maintain compatibility with the SDK's interceptor chain and event emitter pattern. Consider adding validation or documentation to ensure custom implementations follow the expected interface.
Let's verify the usage pattern:
✅ Verification successful
The test file and implementation details will help us understand the expected interface and behavior of custom request handlers, particularly their interaction with interceptors and event emitters.
Based on the code analysis, I can now provide a final response:
Custom request handlers must implement interceptor chain and event emitter pattern
The custom request handler must:
- Accept
optionsobject and spread args as parameters - Process the request after Request interceptors but before Response interceptors
- Maintain compatibility with
options.downloadandoptions.uploadEventEmitters - Return a Promise that resolves with the response
Example usage shown in tests demonstrates proper integration with both patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing request handler implementations to understand the interface
rg -l 'request\s*:\s*function' --type js
# Look for any documentation or examples of request handlers
fd -e md -e js -x grep -l 'request.*function'
Length of output: 925
Script:
#!/bin/bash
# Let's examine the test file to understand the expected interface and behavior
cat packages/@webex/http-core/test/unit/spec/request/index.js
# Also check the main implementation for any interface documentation
cat packages/@webex/http-core/src/request/index.js
# Look for any interceptor chain implementations
rg -A 5 "interceptor" packages/@webex/http-core/
Length of output: 19104
packages/@webex/http-core/test/unit/spec/request/index.js (3)
1-6: LGTM! Well-organized imports.
The imports are clean, purposeful, and include all necessary testing utilities and module dependencies.
8-19: LGTM! Robust test setup with proper cleanup.
The test suite follows best practices with proper initialization of test doubles and cleanup after each test.
1-58: Verify alignment with implementation details.
The tests look comprehensive, but let's verify that all implementation details from the main module are covered.
✅ Verification successful
Let me gather more information about the implementation details to ensure thorough verification.
Let me check the TypeScript utils file and the request implementation to complete the verification.
Tests adequately cover the implementation
The test file correctly verifies all the key aspects of the request implementation:
- URL to URI conversion
- Headers initialization
- EventEmitter instances for download and upload
- Interceptor calls (both for Request and Response phases)
- Custom request function handling
The implementation in index.js and utils.ts aligns perfectly with the test scenarios, and no additional test cases are required as:
- The basic request flow is tested
- The interceptor functionality is verified
- The custom request override is covered
- The options transformation is validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all exported functions from the request module are tested
# and if there are any additional configuration options that should be tested.
# Test: Look for exports and configuration options in the implementation
rg -t js "export|interface|type" packages/@webex/http-core/src/request/
Length of output: 1348
Script:
#!/bin/bash
# Check the actual implementation of the request function and its options
rg -A 10 "export default function request" packages/@webex/http-core/src/request/index.js
# Check for any interceptor-related code that might need testing
rg -A 5 "intercept" packages/@webex/http-core/src/request/utils.js
# Look for any type definitions or interfaces that might indicate required options
fd -e ts -e d.ts . packages/@webex/http-core/src/request/
Length of output: 601
Script:
#!/bin/bash
# Check the utils.ts file for interceptor implementation
cat packages/@webex/http-core/src/request/utils.ts
# Check the complete implementation of request.js
cat packages/@webex/http-core/src/request/index.js
# Check if there are any other test files for request
fd -e spec.js -e test.js . packages/@webex/http-core/test/
Length of output: 3283
| it('should use custom request function if provided', async () => { | ||
| const customRequest = sinon.stub().resolves('custom response'); | ||
| const options = { | ||
| url: 'http://example.com', | ||
| headers: {}, | ||
| interceptors: [], | ||
| request: customRequest, | ||
| }; | ||
|
|
||
| const result = await request(options); | ||
|
|
||
| assert.strictEqual(result, 'custom response'); | ||
| assert.isTrue(customRequest.calledOnceWith(options)); | ||
| assert.isTrue(interceptStub.calledTwice); | ||
| assert.isFalse(requestStub.called); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding edge case tests for custom request handling.
While the happy path is well tested, consider adding the following test cases:
- When custom request function throws an error
- When custom request function is not a function
- When custom request function returns invalid response
Example additional test:
it('should handle errors from custom request function gracefully', async () => {
const customError = new Error('Custom request failed');
const customRequest = sinon.stub().rejects(customError);
const options = {
url: 'http://example.com',
headers: {},
interceptors: [],
request: customRequest
};
try {
await request(options);
assert.fail('Should have thrown an error');
} catch (error) {
assert.strictEqual(error, customError);
assert.isTrue(customRequest.calledOnce);
assert.isTrue(interceptStub.calledTwice);
assert.isFalse(requestStub.called);
}
});
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
robstax
changed the title
robstax
force-pushed
the
options-request
branch
from
48ecb76
to
60c4018
Compare
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-574364
This pull request addresses
particular use case here, where we'd like to use postMessage instead of XHR to interact with a parent page instead of with a HTTP service in certain iframe scenarios
need ability for developers to provide their own request function instead of whatever is coded into SDK
by making the following changes
packages/@webex/http-core/src/request/index.js checks if there is a provided request and if so calls that instead
Change Type
The following scenarios were tested
manually tested with and without custom request
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
requestfunction, enhancing coverage for both default and custom request scenarios.