feat(core): add config for interceptors #3948
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
179-186: Improve documentation of interceptor count calculations.The comments explaining the expected number of interceptors use magic numbers and aren't clearly structured. Consider using a more descriptive format:
- // 4 pre, 4 post, 9 remaining default = 17 + /** + * Default configuration: + * - Pre-interceptors: 4 (default) + * - Post-interceptors: 4 (default) + * - Core interceptors: 9 (default) + * Total expected: 17 + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(1 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🔇 Additional comments (3)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-12: LGTM: Import changes are well-structured.
The addition of PayloadTransformerInterceptor to the imports is properly organized and necessary for the new test cases.
177-261:
Test suite needs improvements in coverage and structure.
While the test cases cover various interceptor configurations, there are several areas that could be improved:
-
The tests only verify the number of interceptors but don't validate:
- The order of interceptors
- The actual functionality of interceptors
- The correct application of pre/post interceptors
-
The test structure could be improved:
- Relies on side effects by modifying defaultOptions
- Ignores request results
- Contains commented-out debug code
Here are the suggested improvements:
- Remove unnecessary code:
- // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors);- Remove redundant defaultOptions initialization:
- let defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
beforeEach(() => {
defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
});- Add additional assertions to verify interceptor order and functionality:
it(message, async () => {
const webex = new WebexCore({
config: {
interceptorsConfig,
},
});
const options = {method: 'GET', url: 'https://fake-url.webex.com'};
await webex.request(options)
.catch(() => {}) // Ignore network errors
.finally(() => {
// Verify number of interceptors
assert.equal(options.interceptors.length, expectedNumberOfInterceptors);
if (interceptorsConfig?.preInterceptors) {
// Verify pre-interceptors are at the start
interceptorsConfig.preInterceptors.forEach((name, index) => {
assert.include(options.interceptors[index].name, name);
});
}
if (interceptorsConfig?.postInterceptors) {
// Verify post-interceptors are at the end
interceptorsConfig.postInterceptors.forEach((name, index) => {
assert.include(
options.interceptors[options.interceptors.length - interceptorsConfig.postInterceptors.length + index].name,
name
);
});
}
});
});Let's verify if there are any existing tests for interceptor functionality:
packages/@webex/webex-core/src/webex-core.js (1)
401-403: Flexibility in Interceptor Configuration
The use of optional chaining and default values allows users to configure interceptors via this.config.interceptorsConfig, enhancing flexibility without breaking existing functionality.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
robstax
force-pushed
the
interceptor-config
branch
from
3ef2359
to
cf0c99a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
251-255: Remove commented console.warn statementsThese debugging statements should be removed as they add noise to the test file.
- // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors); - }) - .catch((error) => { - // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors); + }) + .catch(() => {packages/@webex/webex-core/src/webex-core.js (3)
405-414: Consider adding validation for the interceptor configuration structure.While the implementation is clean and maintains the correct order of interceptors, it might be beneficial to validate the configuration structure to prevent runtime errors. Consider validating that:
- Arrays contain only strings
- Referenced interceptors exist
- No duplicate interceptors across arrays
Example validation:
+const validateInterceptorConfig = (config) => { + if (!config) return; + + const {interceptors: ints, preInterceptors: pre, postInterceptors: post} = config; + + [pre, post].forEach(arr => { + if (arr && (!Array.isArray(arr) || !arr.every(k => typeof k === 'string'))) { + throw new Error('Pre/Post interceptors must be arrays of strings'); + } + }); + + if (ints && typeof ints !== 'object') { + throw new Error('Interceptors must be an object'); + } +}; // Use provided interceptors or default to the existing ones +validateInterceptorConfig(this.config.interceptorsConfig); const interceptorsToUse = this.config.interceptorsConfig?.interceptors || interceptors;
Line range hint
386-396: Fix: Use the provided interceptors object instead of the global one.The
addInterceptorfunction is not using theinterceptorsObjparameter, causing it to always use the globalinterceptorsobject. This prevents custom interceptors from being properly initialized.Apply this fix:
const addInterceptor = (ints, key, interceptorsObj) => { - const interceptor = interceptors[key]; + const interceptor = interceptorsObj[key]; if (!isFunction(interceptor)) { return ints; } ints.push(Reflect.apply(interceptor, this, [])); return ints; };
400-403: Consider adding JSDoc documentation for the interceptor configuration.To improve developer experience, consider adding documentation that describes:
- The structure of
interceptorsConfig- Example configurations
- Best practices for ordering interceptors
Example documentation:
/** * @typedef {Object} InterceptorsConfig * @property {Object} interceptors - Object mapping interceptor names to their implementations * @property {string[]} preInterceptors - Array of interceptor names to run first * @property {string[]} postInterceptors - Array of interceptor names to run last * * @example * { * interceptors: { * CustomInterceptor: CustomInterceptor.create * }, * preInterceptors: ['CustomInterceptor'], * postInterceptors: ['HttpStatusInterceptor'] * } */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(2 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🔇 Additional comments (3)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-12: LGTM: Clean import addition
The PayloadTransformerInterceptor import is properly grouped with related WebexCore imports.
177-236: Well-structured test cases with comprehensive coverage
The test suite provides good coverage of interceptor configuration scenarios:
- Default configuration
- Single interceptor
- Pre-interceptors only
- Post-interceptors only
- Combined configurations
- Empty configuration
The test cases are well-documented with clear descriptions and expected outcomes.
packages/@webex/webex-core/src/webex-core.js (1)
400-403: LGTM! The interceptor configuration implementation looks good.
The implementation correctly allows customization of interceptors while maintaining backward compatibility through default values.
| let defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; | ||
| beforeEach(() => { | ||
| defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; | ||
| }); |
There was a problem hiding this comment.
Remove redundant beforeEach block
The defaultOptions variable is unnecessarily redeclared in the beforeEach block since it's already declared with the same value outside.
-let defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
-beforeEach(() => {
- defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
-});
+const defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};📝 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.
| let defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; | |
| beforeEach(() => { | |
| defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; | |
| }); | |
| const defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; |
| it(message, async () => { | ||
| const webex = new WebexCore({ | ||
| config: { | ||
| interceptorsConfig, | ||
| }, | ||
| }); | ||
| // don't care if it resolves or rejects, just need to see interceptors | ||
| await webex | ||
| .request(defaultOptions) | ||
| .then((result) => { | ||
| // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors); | ||
| }) | ||
| .catch((error) => { | ||
| // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors); | ||
| }) | ||
| .finally(() => { | ||
| assert.equal(defaultOptions.interceptors.length, expectedNumberOfInterceptors); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and readability
The test has several areas for improvement:
- It's using a fake URL that will fail, making the error handling necessary but not meaningful
- The test only verifies the count of interceptors, not their order or functionality
- The code mixes async/await with .then() chains unnecessarily
Consider refactoring to:
- Use a mock request that doesn't actually make a network call
- Verify both count and order of interceptors
- Use consistent async/await style
- it(message, async () => {
- const webex = new WebexCore({
- config: {
- interceptorsConfig,
- },
- });
- // don't care if it resolves or rejects, just need to see interceptors
- await webex
- .request(defaultOptions)
- .then((result) => {
- // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors);
- })
- .catch((error) => {
- // console.warn('### defaultOptions.interceptors', defaultOptions.interceptors);
- })
- .finally(() => {
- assert.equal(defaultOptions.interceptors.length, expectedNumberOfInterceptors);
- });
+ it(message, async () => {
+ const webex = new WebexCore({
+ config: {
+ interceptorsConfig,
+ },
+ });
+
+ // Mock the request to avoid network call
+ sinon.stub(webex, 'request').resolves({});
+
+ await webex.request(defaultOptions);
+
+ // Verify interceptor count
+ assert.equal(defaultOptions.interceptors.length, expectedNumberOfInterceptors);
+
+ // Verify interceptor order if applicable
+ if (interceptorsConfig?.preInterceptors) {
+ const preInterceptorNames = defaultOptions.interceptors
+ .slice(0, interceptorsConfig.preInterceptors.length)
+ .map(i => i.name);
+ assert.deepEqual(preInterceptorNames, interceptorsConfig.preInterceptors);
+ }Committable suggestion was skipped due to low confidence.
robstax
force-pushed
the
interceptor-config
branch
from
cf0c99a
to
c04170f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
179-204: Add JSDoc to document test data structureThe test data structure would benefit from documentation to improve maintainability.
Add JSDoc comment to describe the test data structure:
+/** + * Test cases for interceptor initialization + * @type {Array<[string, object|undefined, number, string[]]>} + * Format: [testDescription, interceptorConfig, expectedCount, expectedInterceptorNames] + */ [ [ 'defaults to existing interceptors if undefined', undefined, 17, [ 'RequestTimingInterceptor', // ... ], ], // ... ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(1 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/webex-core/src/webex-core.js
🔇 Additional comments (2)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-13: LGTM: Import changes are well-structured
The additional interceptor imports are properly organized within the existing WebexCore imports.
232-235:
Remove redundant beforeEach block
This issue was previously identified. The defaultOptions variable is unnecessarily redeclared in the beforeEach block.
Move the declaration outside the test cases:
- let defaultOptions;
- beforeEach(() => {
- defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
- });
+ const defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'};
robstax
force-pushed
the
interceptor-config
branch
from
c04170f
to
99dac2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
179-204: Verify interceptor initialization orderThe test cases verify the presence and count of interceptors but don't explicitly verify the initialization order of pre/post interceptors.
Consider adding assertions to verify that pre-interceptors are added before core interceptors, and post-interceptors are added after:
// Add to test cases array [ 'verifies interceptor order', { preInterceptors: ['RequestTimingInterceptor'], interceptors: { PayloadTransformerInterceptor: PayloadTransformerInterceptor.create }, postInterceptors: ['RequestEventInterceptor'] }, 3, ['RequestTimingInterceptor', 'PayloadTransformerInterceptor', 'RequestEventInterceptor'] ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(2 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🔇 Additional comments (4)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-13: LGTM: Import changes align with PR objectives
The additional imports for interceptor classes are properly structured and necessary for testing the new interceptor configuration functionality.
232-235: Remove redundant beforeEach block
The defaultOptions variable declaration can be moved outside the test cases as it's constant and doesn't need to be reset for each test.
packages/@webex/webex-core/src/webex-core.js (2)
387-388: LGTM! Dynamic interceptor configuration implemented correctly.
The changes allow for custom interceptor configuration while maintaining backward compatibility through the fallback mechanism.
401-409: 🛠️ Refactor suggestion
Consider adding validation and documentation for interceptor configuration.
While the implementation is functional, consider these improvements:
- Add validation for the custom interceptor configuration to ensure it follows the expected structure.
- Document the required format and any ordering requirements for custom interceptors.
- Consider adding a warning when overriding critical interceptors.
Let's check if there's documentation about the interceptor structure:
Consider adding validation like this:
if (this.config.interceptors) {
+ // Validate interceptor configuration
+ const requiredInterceptors = ['AuthInterceptor', 'HttpStatusInterceptor'];
+ const configuredInterceptors = Object.keys(this.config.interceptors);
+
+ requiredInterceptors.forEach(interceptor => {
+ if (!configuredInterceptors.includes(interceptor)) {
+ console.warn(`Warning: Required interceptor '${interceptor}' is not configured`);
+ }
+ });
+
Object.keys(this.config.interceptors).reduce(addInterceptor, ints);
} else {
ints = preInterceptors.reduce(addInterceptor, ints);| describe('initializes with interceptors', () => { | ||
| [ | ||
| // 4 pre, 4 post, 9 remaining default = 17 | ||
| [ | ||
| 'defaults to existing interceptors if undefined', | ||
| undefined, | ||
| 17, | ||
| [ | ||
| 'RequestTimingInterceptor', | ||
| 'RequestEventInterceptor', | ||
| 'WebexTrackingIdInterceptor', | ||
| 'RateLimitInterceptor', | ||
| 'ServiceInterceptor', | ||
| 'UserAgentInterceptor', | ||
| 'WebexUserAgentInterceptor', | ||
| 'AuthInterceptor', | ||
| 'PayloadTransformerInterceptor', | ||
| 'RedirectInterceptor', | ||
| 'DefaultOptionsInterceptor', | ||
| 'HostMapInterceptor', | ||
| 'ServerErrorInterceptor', | ||
| 'HttpStatusInterceptor', | ||
| 'NetworkTimingInterceptor', | ||
| 'EmbargoInterceptor', | ||
| 'RateLimitInterceptor', | ||
| ], | ||
| ], | ||
| [ | ||
| 'only adds PayloadTransformerInterceptor', | ||
| {interceptors: {PayloadTransformerInterceptor: PayloadTransformerInterceptor.create}}, | ||
| 1, | ||
| ['PayloadTransformerInterceptor'], | ||
| ], | ||
| [ | ||
| 'adds multiple interceptors', | ||
| { | ||
| interceptors: { | ||
| PayloadTransformerInterceptor: PayloadTransformerInterceptor.create, | ||
| RequestTimingInterceptor: RequestTimingInterceptor.create, | ||
| }, | ||
| }, | ||
| 2, | ||
| ['PayloadTransformerInterceptor', 'RequestTimingInterceptor'], | ||
| ], | ||
| [ | ||
| 'adds no interceptors', | ||
| { | ||
| interceptors: [], | ||
| }, | ||
| 0, | ||
| [], | ||
| ], | ||
| ].forEach( | ||
| ([message, interceptorsConfig, expectedNumberOfInterceptors, expectedInterceptorNames]) => { | ||
| let defaultOptions; | ||
| beforeEach(() => { | ||
| defaultOptions = {method: 'GET', url: 'https://fake-url.webex.com'}; | ||
| }); | ||
| it(message, async () => { | ||
| const webex = new WebexCore({ | ||
| config: interceptorsConfig, | ||
| }); | ||
| // don't care if it resolves or rejects, just need to see interceptors | ||
| try { | ||
| await webex.request(defaultOptions); | ||
| } catch (error) { | ||
| } finally { | ||
| assert.equal(defaultOptions.interceptors.length, expectedNumberOfInterceptors); | ||
| assert.deepEqual( | ||
| defaultOptions.interceptors.map((int) => int.constructor.name), | ||
| expectedInterceptorNames | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding negative test cases
The test suite focuses on valid configurations but doesn't test error cases.
Consider adding test cases for:
- Invalid interceptor names
- Malformed interceptor configurations
- Duplicate interceptor definitions
Example:
[
'throws error for invalid interceptor name',
{
interceptors: {
NonExistentInterceptor: () => {}
}
},
Error,
'Invalid interceptor configuration'
]There was a problem hiding this comment.
not concerned with those right now. if developer wants to foot gun themselves, that's on them
There was a problem hiding this comment.
@robstax, understood. Let me know if there's anything else I can assist with.
✏️ Learnings added
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.253Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
robstax
force-pushed
the
interceptor-config
branch
from
99dac2a
to
bf69afe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(2 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/webex-core/src/webex-core.js
🧰 Additional context used
📓 Learnings (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
🔇 Additional comments (2)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-13: LGTM: Import statements are well-structured
The new imports for interceptors are properly organized within the existing import block from '@webex/webex-core'.
178-229: LGTM: Well-structured test cases with good coverage
The test suite effectively covers various interceptor configurations:
- Default interceptors
- Single interceptor
- Multiple interceptors
- Empty interceptors array
The test cases are well-organized and provide clear descriptions of what's being tested.
robstax
force-pushed
the
interceptor-config
branch
from
bf69afe
to
694036e
Compare
robstax
force-pushed
the
interceptor-config
branch
from
694036e
to
8c88535
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/webex-core/src/webex-core.js (1)
Line range hint
387-409: Overall implementation looks good with minor suggestions.The changes successfully implement configurable interceptors while maintaining backward compatibility. The implementation is clean and localized to the interceptor initialization logic, minimizing the impact on the rest of the codebase.
Consider documenting the interceptor configuration structure in the class's JSDoc comments to help users understand how to properly configure interceptors.
🛑 Comments failed to post (1)
packages/@webex/webex-core/src/webex-core.js (1)
401-409:
⚠️ Potential issuePreserve interceptor ordering when using custom configuration.
The current implementation doesn't maintain the pre/post interceptor ordering when using custom configuration. This could lead to unexpected behavior if the order of interceptors is important for the application.
Consider preserving the ordering:
if (this.config.interceptors) { - Object.keys(this.config.interceptors).reduce(addInterceptor, ints); + // Apply pre-interceptors first + ints = preInterceptors + .filter((key) => this.config.interceptors[key]) + .reduce(addInterceptor, ints); + // Apply main interceptors + ints = Object.keys(this.config.interceptors) + .filter((key) => !(preInterceptors.includes(key) || postInterceptors.includes(key))) + .reduce(addInterceptor, ints); + // Apply post-interceptors last + ints = postInterceptors + .filter((key) => this.config.interceptors[key]) + .reduce(addInterceptor, ints); } else {📝 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.if (this.config.interceptors) { // Apply pre-interceptors first ints = preInterceptors .filter((key) => this.config.interceptors[key]) .reduce(addInterceptor, ints); // Apply main interceptors ints = Object.keys(this.config.interceptors) .filter((key) => !(preInterceptors.includes(key) || postInterceptors.includes(key))) .reduce(addInterceptor, ints); // Apply post-interceptors last ints = postInterceptors .filter((key) => this.config.interceptors[key]) .reduce(addInterceptor, ints); } else { ints = preInterceptors.reduce(addInterceptor, ints); ints = Object.keys(interceptors) .filter((key) => !(preInterceptors.includes(key) || postInterceptors.includes(key))) .reduce(addInterceptor, ints); ints = postInterceptors.reduce(addInterceptor, ints); }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
180-203: Consider these improvements for better test reliability
- Add a comment explaining the magic number 17 in the default interceptors test case
- Log the error in the catch block for better debugging:
try { await webex.request(requestOptions); } catch (error) { + // Log error but continue as we're only interested in interceptor setup + console.log('Expected request error:', error.message); } finally {
- Consider adding a comment explaining why we continue after the error:
// 4 pre, 4 post, 9 remaining default = 17 +// Note: We allow the request to fail since we're only verifying +// that interceptors are properly initialized, not their execution [ 'defaults to existing interceptors if undefined', undefined,Also applies to: 237-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/webex-core/src/webex-core.js(2 hunks)packages/@webex/webex-core/test/unit/spec/webex-core.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/webex-core/src/webex-core.js
🧰 Additional context used
📓 Learnings (1)
packages/@webex/webex-core/test/unit/spec/webex-core.js (1)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
🔇 Additional comments (2)
packages/@webex/webex-core/test/unit/spec/webex-core.js (2)
7-13: LGTM: Import changes are well-organized
The new interceptor imports are properly grouped with other WebexCore imports and are necessary for the test cases.
178-250: LGTM: Well-structured test suite with comprehensive coverage
The test suite effectively covers various interceptor configurations:
- Default interceptors
- Single interceptor
- Multiple interceptors
- Empty interceptors
Good use of table-driven tests and clear test descriptions.
COMPLETES
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-576500
This pull request addresses
right now the interceptors in the SDK are hardcoded. this is a non-breaking change, but consumers of webex-js-sdk should be able to config these.
by making the following changes
allow sdk config to pass in an object describing the interceptors. to match the logic and format of the existing webex-core plugin the share is:
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
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
PayloadTransformerInterceptorandRequestTimingInterceptor, enhancing functionality.Bug Fixes
Documentation