feat: allow to set symbol property on request, response, context and app #12
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several modifications across the project, focusing on enhancing flexibility and expanding compatibility. The changes include updating the ESLint configuration to extend additional rules, expanding the Node.js CI testing matrix to include version 23, adding a Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant MW as Middleware
participant Req as Request
participant Res as Response
participant Ctx as Context
App->>MW: use middleware
MW-->>App: register middleware
App->>Req: create request
App->>Res: create response
App->>Ctx: create context
Ctx->>Req: delegate methods
Ctx->>Res: delegate methods
App->>MW: compose and execute middleware
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 1714 1718 +4
Branches 315 319 +4
=========================================
+ Hits 1714 1718 +4 ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (6)
example/extend/Request.ts (1)
6-18: Add JSDoc documentationConsider adding JSDoc to document the host getter's behavior, especially regarding proxy handling.
Add documentation like this:
+ /** + * Get the host of the request. + * When app.proxy is true, supports X-Forwarded-Host + * @return {string} hostname:port + */ get host(): string {src/context.ts (1)
14-14: Add documentation for symbol property usageWhile the implementation is type-safe using
unknown, it would be helpful to add JSDoc comments explaining:
- The intended use cases for symbol properties
- Best practices for symbol management
- Memory management considerations
+/** + * @description Allows setting custom properties using symbols as keys + * @example + * const customProp = Symbol('customProp'); + * ctx[customProp] = value; + */ [key: symbol]: unknown;src/application.ts (1)
32-32: Add documentation for symbol property usageFor consistency with the Context class, please add documentation for the symbol property feature.
+/** + * @description Allows setting custom properties using symbols as keys + * @example + * const customProp = Symbol('customProp'); + * app[customProp] = value; + */ [key: symbol]: unknown;src/response.ts (2)
20-20: Add documentation for symbol property usageFor consistency with other core classes, please add documentation for the symbol property feature.
+/** + * @description Allows setting custom properties using symbols as keys + * @example + * const customProp = Symbol('customProp'); + * response[customProp] = value; + */ [key: symbol]: unknown;
Line range hint
14-14: Consider adding comprehensive documentation for symbol property featureThe addition of symbol property support across core classes (Context, Application, and Response) is well-implemented with type safety in mind. However, consider adding:
- A central documentation section explaining:
- The motivation behind this feature
- Guidelines for creating and managing symbols
- Best practices for symbol property lifecycle
- Example use cases in the README or documentation
- Performance implications and memory management considerations
This will help users understand and properly utilize this new feature.
Also applies to: 32-32, 20-20
src/request.ts (1)
16-16: Consider adding documentation for symbol property usage.Since this introduces a new way to store properties on the Request class, it would be helpful to:
- Document the intended use cases
- Add examples of proper symbol property management
- Note any cleanup requirements to prevent memory leaks
Example documentation to add above the class:
/** * ...existing class documentation... * * Symbol Properties: * The Request class supports symbol-based properties for storing metadata: * * ```typescript * const metadataSymbol = Symbol('metadata'); * req[metadataSymbol] = { /* custom data */ }; * ``` * * Note: Ensure proper cleanup of symbol properties when they are no longer needed * to prevent memory leaks in long-running applications. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.eslintrc(1 hunks).github/workflows/node.js.yml(1 hunks)example/extend/Request.ts(1 hunks)example/helloworld.cjs(1 hunks)example/helloworld.mjs(1 hunks)src/application.ts(2 hunks)src/context.ts(1 hunks)src/request.ts(1 hunks)src/response.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- example/helloworld.cjs
- example/helloworld.mjs
🔇 Additional comments (6)
.eslintrc (1)
3-4: LGTM! Verify rule compatibility
The addition of enforce-node-prefix rule aligns with the PR's objective of handling symbol properties consistently.
✅ Verification successful
Rule enforces node: prefix for Node.js built-in modules
The rule is correctly implemented and enforces the use of the node: protocol prefix for Node.js built-in modules (like node:fs, node:path, etc.) by using ESLint's no-restricted-imports and no-restricted-modules rules. This is a recommended practice for explicit module resolution in Node.js. The rule is compatible with the project's eslint-config-egg v14.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the eslint rule exists and is properly documented
curl -s https://raw.githubusercontent.com/eggjs/eslint-config-egg/master/lib/rules/enforce-node-prefix.js | grep -q "module.exports" && echo "Rule exists" || echo "Rule not found"
Length of output: 165
Script:
#!/bin/bash
# Let's verify the actual content of the rule to understand its implementation
curl -s https://raw.githubusercontent.com/eggjs/eslint-config-egg/master/lib/rules/enforce-node-prefix.js
# Also check if there are any existing usages of this rule in the codebase
rg -l "node\." -t js -t ts
# And check package.json for eslint-config-egg version
cat package.json | jq '.dependencies["eslint-config-egg"] // .devDependencies["eslint-config-egg"]'
Length of output: 1342
.github/workflows/node.js.yml (1)
15-15: Consider Node.js version stability implications
Adding Node.js 23 expands test coverage to include the latest version. However, note that v23 is not an LTS release and may have stability implications.
✅ Verification successful
Node.js version matrix is compatible with package requirements
The package.json specifies >= 18.19.0 as the Node.js engine requirement, which aligns well with the CI test matrix (18.19.0, 18, 20, 22, 23). No version-specific APIs or features were found in the codebase that could cause stability issues. While Node.js 23 is not LTS, testing against it provides early feedback for future compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for engine constraints
fd package.json --exec sh -c 'echo "Checking engine constraints in {}:"; cat {} | jq -r ".engines.node"'
Length of output: 164
Script:
#!/bin/bash
# Check for Node.js version-specific features or APIs in the codebase
rg -i --type js --type ts 'node:[0-9]+' -A 2
# Look for potential version-sensitive Node.js APIs
rg -i --type js --type ts '(webcrypto|test_runner|--test|fetch|WebSocket|diagnostics_channel|worker_threads|performance\.timeOrigin|--experimental)' -A 2
Length of output: 202
example/extend/Request.ts (2)
3-3: LGTM! Good use of Symbol for private property
Using Symbol for the host cache prevents naming collisions and aligns with the PR's objective.
5-19: Verify consistency with existing patterns
Let's ensure this implementation aligns with existing patterns in the codebase.
✅ Verification successful
Let me gather more specific information about getter patterns in the codebase.
Based on the comprehensive results, I can now generate the final response:
Implementation follows existing patterns correctly
The host getter implementation in CustomRequest follows the established patterns in the codebase:
- Uses Symbol for private storage (
HOST) consistent with other Symbol usage - Follows the getter pattern seen throughout the codebase
- Handles proxy cases similar to the base implementation
- Uses the same string splitting pattern for handling multiple hosts
The main difference is the default to '127.0.0.1' when proxy is enabled, which is an intentional customization that doesn't break the overall pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar Symbol usage patterns
ast-grep --pattern 'const $_ = Symbol($$$)'
# Check for similar getter implementations
ast-grep --pattern 'get $_() {
$$$
return $$$;
}'
Length of output: 166
Script:
#!/bin/bash
# Search for getter implementations in Request-related files
rg "get \w+\(\)" -A 10
# Check for host-related patterns
rg "host" -A 5
Length of output: 42178
src/application.ts (1)
20-20: LGTM: Debug namespace update
The debug namespace change to '@eggjs/koa/application' improves consistency with the package structure.
src/request.ts (1)
16-16: LGTM! Well-designed symbol property support.
The addition of symbol-based property support is a good design choice:
- Uses TypeScript's
unknowntype for type-safety - Prevents naming collisions via symbols
- Enables plugins/middleware to safely store metadata
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
[skip ci] ## [2.20.0](v2.19.2...v2.20.0) (2024-12-18) ### Features * allow to set symbol property on request, response, context and app ([#12](#12)) ([5e18ed2](5e18ed2))
Summary by CodeRabbit
New Features
CustomRequestclass for enhanced host value retrieval.Improvements
Applicationclass.Context,Request, andResponseclasses for increased flexibility.Refactor