chore(common): apply decorators function #3999
Conversation
WalkthroughThe changes introduce a new function Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Device
participant Decorators
User->>Device: Calls method (e.g., register)
Device->>Decorators: Applies decorators
Decorators->>Device: Modifies method behavior
Device->>User: Returns result
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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: 3
🧹 Outside diff range and nitpick comments (3)
packages/@webex/common/src/apply-decorators.js (1)
1-8: Improve JSDoc documentation accuracy and completeness.The JSDoc comments should be enhanced with more specific type information and a better description of the
decoratedMethodsparameter structure./** * This applies decorators to an ampersand state object * It wraps the function in the order the decorators are provided * This means that when you call the method, the decorators are applied in reverse order - * @param {*} object - The object to apply the decorators to - * @param {*} decoratedMethods - * @returns {undefined} + * @param {Object} object - The object to apply the decorators to + * @param {Object.<string, Function[]>} decoratedMethods - Map of method names to their decorator functions */packages/@webex/common/test/unit/spec/apply-decorators.js (1)
25-33: Document decorator application orderWhile the test correctly verifies the decorator order, it would be helpful to add a comment explaining that decorators are applied in reverse order (last decorator wraps first). This is a common source of confusion in decorator implementations.
applyDecorators(obj, { + // Decorators are applied in reverse order: + // decorator2 wraps decorator1, which wraps the original method method: [decorator1, decorator2] });packages/@webex/internal-plugin-device/src/device.js (1)
370-370: Great improvement in parameter naming!The change from
optionstodeviceRegistrationOptionsenhances code clarity by making the expected type more explicit while maintaining backward compatibility.Consider adding TypeScript-style JSDoc type annotations to further improve type safety:
- @param {Object} deviceRegistrationOptions - The options for registration. + @param {Object} deviceRegistrationOptions - The options for registration. + @typedef {Object} DeviceRegistrationOptions + @property {CatalogDetails} [includeDetails] - The details to include in the refresh/register request.Also applies to: 446-446
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/common/src/apply-decorators.js(1 hunks)packages/@webex/common/src/index.js(1 hunks)packages/@webex/common/test/unit/spec/apply-decorators.js(1 hunks)packages/@webex/internal-plugin-device/src/device.js(4 hunks)
🔇 Additional comments (7)
packages/@webex/common/src/apply-decorators.js (2)
1-20: Verify that this resolves the VSCode linting issues.
Since this PR aims to fix VSCode linting issues with decorators on ampersand state objects, please confirm:
- Have you tested this with VSCode to ensure the linting errors are resolved?
- Are there any specific VSCode settings or extensions that need to be documented?
✅ Verification successful
The implementation should resolve VSCode linting issues
Based on the codebase analysis:
- The repository has TypeScript configuration with
experimentalDecoratorsenabled, which is the correct setting for decorator support. - The implementation moves away from using decorators directly on classes to using a programmatic approach with
applyDecorators, which is a common pattern to avoid linting issues. - The change is consistent with the codebase's approach to handling decorators in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining decorator usage that might still cause linting issues
rg -t js "@\w+\s*\n*\s*class"
rg -t js "@\w+\s*\n*\s*\w+\s*\([^)]*\)\s*{"
Length of output: 397
Script:
#!/bin/bash
# Let's try a different approach to find decorator usage
# Search for @ symbol followed by word characters
rg -U "@\w+" --type js
# Also check for any configuration files that might affect VSCode linting
fd -e json -e js "\.eslint" -X cat {}
Length of output: 130818
Script:
#!/bin/bash
# Let's check for any ESLint configuration files that might affect decorator usage
fd -e json -e js -e yaml -e yml "eslint|\.eslintrc" -X cat {}
# Also check for any VSCode settings that might be relevant
fd -g "settings.json" -X cat {}
# And check for any TypeScript configuration that might affect decorators
fd -g "tsconfig*.json" -X cat {}
Length of output: 10131
9-16: Consider preserving property descriptors.
The current implementation might not properly handle property descriptors, which could affect how decorators behave with getters/setters or non-configurable properties.
Let's check if there are any getters/setters in the codebase that might be decorated:
packages/@webex/common/test/unit/spec/apply-decorators.js (2)
7-23: Well-structured test setup with proper function binding
The test setup is well-designed:
- Uses sinon stub for method tracking
- Correctly implements decorators with proper 'this' binding
- Clear demonstration of decorator behavior
1-3: Verify the new utility module's structure
The import path suggests this is a new shared utility. Let's verify its structure and exports.
✅ Verification successful
Import structure and module location are correct
The utility module exists at the expected location with proper exports. The implementation shows a well-documented utility function for applying decorators to ampersand state objects, and it's currently only imported by its test file. The import structure in the test file is consistent with the module's exports and follows the project's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new utility module structure and exports
# Check if the module exists and exports the function
rg -g "src/apply-decorators.js" -A 5 "export.*applyDecorators"
# Check if there are any other files importing this utility
rg "from ['\"]\@webex\/common\/src\/apply-decorators['\"]"
Length of output: 403
Script:
#!/bin/bash
# Let's try a different approach to verify the module structure
# Find the apply-decorators.js file
fd "apply-decorators.js"
# Check the content of the file if found
fd "apply-decorators.js" --exec cat {}
# Search for imports using ripgrep with proper escaping
rg "from ['\"]@webex/common/src/apply-decorators['\"]"
Length of output: 1789
packages/@webex/common/src/index.js (1)
24-24: LGTM! The export is well-placed and follows consistent patterns.
The new export for applyDecorators is correctly positioned among related utilities and maintains the file's organization.
Let's verify the implementation of the applyDecorators utility:
✅ Verification successful
Implementation and usage of applyDecorators looks solid and serves its purpose
The implementation in apply-decorators.js is well-designed:
- Takes an object and a map of method-to-decorators
- Correctly applies multiple decorators in the expected order (reverse execution)
- Has proper JSDoc documentation
Current usage shows:
- Used in
@webex/internal-plugin-device/src/device.jsto apply decorators programmatically - Has unit tests in
@webex/common/test/unit/spec/apply-decorators.js - The codebase shows extensive use of decorators that could benefit from this utility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of applyDecorators
# Check the implementation
echo "Checking applyDecorators implementation:"
cat packages/@webex/common/src/apply-decorators.js
# Find usages to understand the impact
echo -e "\nChecking usages across the codebase:"
rg "applyDecorators" --type js
# Look for any existing decorator-related issues
echo -e "\nChecking for existing decorator patterns that might need migration:"
rg -B2 -A2 "@\w+\s*\([^)]*\)\s*$" --type js
Length of output: 12983
packages/@webex/internal-plugin-device/src/device.js (2)
2-2: LGTM! Import changes align with PR objectives
The addition of applyDecorators import supports the goal of manually applying decorators to resolve VSCode linting issues.
886-894: Well-structured decorator application!
The centralized application of decorators using applyDecorators:
- Resolves VSCode linting issues as intended
- Improves code maintainability by grouping all decorators in one place
- Makes decorator application more explicit and easier to manage
Let's verify that all decorated methods are properly exported and accessible:
✅ Verification successful
Decorator application correctly matches implemented methods
The verification confirms that all methods decorated in the applyDecorators call are properly implemented in the Device class:
register(deviceRegistrationOptions = {})refresh(deviceRegistrationOptions = {})unregister()markUrlFailedAndGetNew(url)initialize(...args)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all decorated methods exist in the Device class
ast-grep --pattern 'class Device {
$$$
register($$$) {
$$$
}
refresh($$$) {
$$$
}
unregister($$$) {
$$$
}
markUrlFailedAndGetNew($$$) {
$$$
}
initialize($$$) {
$$$
}
$$$
}'
Length of output: 218
Script:
#!/bin/bash
# Let's try a different approach to verify the methods individually
echo "=== Checking register method ==="
ast-grep --pattern 'register($$$) {
$$$
}'
echo "=== Checking refresh method ==="
ast-grep --pattern 'refresh($$$) {
$$$
}'
echo "=== Checking unregister method ==="
ast-grep --pattern 'unregister($$$) {
$$$
}'
echo "=== Checking markUrlFailedAndGetNew method ==="
ast-grep --pattern 'markUrlFailedAndGetNew($$$) {
$$$
}'
echo "=== Checking initialize method ==="
ast-grep --pattern 'initialize($$$) {
$$$
}'
# Also let's check the Device class definition
echo "=== Checking Device class ==="
ast-grep --pattern 'class Device extends $$$'
Length of output: 777
Script:
#!/bin/bash
# Let's try using ripgrep to search for these methods and class
echo "=== Checking Device class and methods ==="
rg -A 2 "class Device" packages/@webex/internal-plugin-device/src/device.js
echo "=== Checking method implementations ==="
rg "(register|refresh|unregister|markUrlFailedAndGetNew|initialize)\s*\([^)]*\)\s*{" packages/@webex/internal-plugin-device/src/device.js -A 2
Length of output: 626
Script:
#!/bin/bash
# Let's try a simpler ripgrep approach
echo "=== Checking Device class ==="
rg "class Device" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "=== Checking individual methods ==="
echo "--- register ---"
rg "register.*{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- refresh ---"
rg "refresh.*{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- unregister ---"
rg "unregister.*{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- markUrlFailedAndGetNew ---"
rg "markUrlFailedAndGetNew.*{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- initialize ---"
rg "initialize.*{" packages/@webex/internal-plugin-device/src/device.js -A 2
Length of output: 1342
Script:
#!/bin/bash
# Let's try with escaped curly braces in the regex
echo "=== Checking Device class ==="
rg "class Device" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "=== Checking individual methods ==="
echo "--- register ---"
rg "register.*\{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- refresh ---"
rg "refresh.*\{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- unregister ---"
rg "unregister.*\{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- markUrlFailedAndGetNew ---"
rg "markUrlFailedAndGetNew.*\{" packages/@webex/internal-plugin-device/src/device.js -A 2
echo "--- initialize ---"
rg "initialize.*\{" packages/@webex/internal-plugin-device/src/device.js -A 2
Length of output: 2010
| export default { | ||
| applyDecorators, | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Consider removing default export since only named export is used
The codebase exclusively uses the named import {applyDecorators}, making the default export unnecessary. For better code maintainability and to avoid confusion, consider removing the default export and keeping only the named export.
packages/@webex/common/src/apply-decorators.js: Remove default export and keep onlyexport { applyDecorators }
🔗 Analysis chain
Verify import usage consistency across the codebase.
Having both named and default exports is fine, but let's ensure they're used consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this module is imported across the codebase
rg -t js "import.*from.*apply-decorators"
Length of output: 169
| export function applyDecorators(object, decoratedMethods) { | ||
| Object.entries(decoratedMethods).forEach(([method, decorators]) => { | ||
| object[method] = decorators.reduce( | ||
| (decorated, decorator) => decorator(decorated), | ||
| object[method] | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add input validation to prevent runtime errors.
The function should validate its inputs to ensure robust operation and provide clear error messages.
export function applyDecorators(object, decoratedMethods) {
+ if (!object || typeof object !== 'object') {
+ throw new TypeError('First argument must be an object');
+ }
+ if (!decoratedMethods || typeof decoratedMethods !== 'object') {
+ throw new TypeError('Second argument must be an object mapping methods to decorators');
+ }
+
Object.entries(decoratedMethods).forEach(([method, decorators]) => {
+ if (!Array.isArray(decorators)) {
+ throw new TypeError(`Decorators for method "${method}" must be an array`);
+ }
+ if (typeof object[method] !== 'function') {
+ throw new TypeError(`"${method}" is not a method on the target object`);
+ }
+ if (!decorators.every((d) => typeof d === 'function')) {
+ throw new TypeError(`All decorators for method "${method}" must be functions`);
+ }
+
object[method] = decorators.reduce(
(decorated, decorator) => decorator(decorated),
object[method]
);
});
}📝 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.
| export function applyDecorators(object, decoratedMethods) { | |
| Object.entries(decoratedMethods).forEach(([method, decorators]) => { | |
| object[method] = decorators.reduce( | |
| (decorated, decorator) => decorator(decorated), | |
| object[method] | |
| ); | |
| }); | |
| } | |
| export function applyDecorators(object, decoratedMethods) { | |
| if (!object || typeof object !== 'object') { | |
| throw new TypeError('First argument must be an object'); | |
| } | |
| if (!decoratedMethods || typeof decoratedMethods !== 'object') { | |
| throw new TypeError('Second argument must be an object mapping methods to decorators'); | |
| } | |
| Object.entries(decoratedMethods).forEach(([method, decorators]) => { | |
| if (!Array.isArray(decorators)) { | |
| throw new TypeError(`Decorators for method "${method}" must be an array`); | |
| } | |
| if (typeof object[method] !== 'function') { | |
| throw new TypeError(`"${method}" is not a method on the target object`); | |
| } | |
| if (!decorators.every((d) => typeof d === 'function')) { | |
| throw new TypeError(`All decorators for method "${method}" must be functions`); | |
| } | |
| object[method] = decorators.reduce( | |
| (decorated, decorator) => decorator(decorated), | |
| object[method] | |
| ); | |
| }); | |
| } |
| describe('applyDecorators()', () => { | ||
| it('applies the decorators in order', () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases
The test suite currently only verifies the order of decorator application. Given that this PR addresses VSCode issues with ampersand state objects, consider adding test cases for:
- Error handling scenarios
- Edge cases (e.g., empty decorator array, undefined methods)
- Integration with ampersand state objects
Would you like me to help generate these additional test cases?
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
Decorators cause lots of errors in VScode due to the way they are trying to apply to ampersand state objects.
by making the following changes
I've created a new function to apply the decorators manually. This avoids the need to physically decorate the code and so the linting works again.
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
New Features
applyDecoratorsfunction for applying decorators to methods, enhancing functionality.Deviceclass with centralized decorator application for improved readability.Bug Fixes
refreshandregisterfor clearer parameter naming.Tests
applyDecoratorsfunction to ensure correct functionality.