fix: remove delegates from Context #15
Conversation
WalkthroughThis pull request introduces significant architectural changes to the Koa framework, focusing on enhancing type safety and removing the previous delegation mechanism. The modifications span multiple files, with key changes including the introduction of generic type parameters, direct method and property definitions in the Changes
Sequence DiagramsequenceDiagram
participant App as Application<T>
participant Ctx as Context
participant Req as Request
participant Res as Response
App->>Ctx: Create Context
Ctx->>Req: Initialize Request
Ctx->>Res: Initialize Response
Ctx-->>App: Return Typed Context
App->>Ctx: Apply Middleware
Ctx->>Res: Manipulate Response
Possibly Related PRs
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 (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@20.17.11, npm/delegates@1.0.0, npm/mm@3.4.0 |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15 +/- ##
===========================================
- Coverage 100.00% 98.44% -1.56%
===========================================
Files 6 6
Lines 1725 1924 +199
Branches 321 366 +45
===========================================
+ Hits 1725 1894 +169
- Misses 0 30 +30 ☔ 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: 0
🧹 Nitpick comments (6)
src/application.ts (3)
154-154: Typed middleware application
Ensuringfnis a function and rejecting generator logic is consistent with modern async/await usage. Consider adding a test case to confirm that passing a non-function throws the correct error.
222-222: Context creation method
Currently annotated as@privatebut exposed as a public method. Consider aligning the visibility modifier with the intended usage, or clarify in docs if it’s deliberately public for advanced use cases.
249-249: Protected_respond
The method systematically handles various response types (buffer, string, stream, JSON). It's lengthy, but logically sound. For future maintainability, consider factoring out specialized blocks (e.g., HEAD handling) into smaller helper methods.test/test-helpers/context.ts (1)
2-2: Switched to named import
RenamingApplicationasKoaclarifies the usage but keep an eye on consistency with the rest of the project to avoid confused references.test/application/context.test.ts (1)
42-67: Comprehensive test coverage forMyContextandMyApp.
These tests nicely demonstrate how subclassingContextand providing a customApplicationsubclass can add extra functionality. Consider adding negative or edge-case tests (e.g., verifying error handling) to further validate behavior.docs/api/response.md (1)
Line range hint
216-222: Enhance documentation with more detailed examples and behavior description.While the documentation follows the consistent style, it could be more comprehensive. Consider adding:
- Example of appending multiple values
- Behavior when the header already exists
- Relationship with the
setmethodHere's a suggested expansion:
### response.append(field, value) Append additional header `field` with value `val`. + + If the header already exists, the new value will be appended to the existing one. + For multiple values, you can call append multiple times: + +```js +// Appending multiple values +ctx.append('Link', '<http://127.0.0.1/>'); +ctx.append('Link', '<http://example.com/>'); +// Results in: Link: <http://127.0.0.1/>, <http://example.com/> + +// Appending array of values +ctx.append('Vary', ['Accept', 'User-Agent']); +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/api/response.md(1 hunks)example/extend/middleware.ts(2 hunks)package.json(3 hunks)src/application.ts(8 hunks)src/context.ts(2 hunks)src/request.ts(3 hunks)src/response.ts(3 hunks)test/application.test-d.ts(1 hunks)test/application/context.test.ts(2 hunks)test/test-helpers/context.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/context.ts
[warning] 273-274: src/context.ts#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 337-338: src/context.ts#L337-L338
Added lines #L337 - L338 were not covered by tests
[warning] 341-342: src/context.ts#L341-L342
Added lines #L341 - L342 were not covered by tests
[warning] 345-346: src/context.ts#L345-L346
Added lines #L345 - L346 were not covered by tests
[warning] 349-350: src/context.ts#L349-L350
Added lines #L349 - L350 were not covered by tests
[warning] 353-354: src/context.ts#L353-L354
Added lines #L353 - L354 were not covered by tests
[warning] 365-366: src/context.ts#L365-L366
Added lines #L365 - L366 were not covered by tests
[warning] 377-378: src/context.ts#L377-L378
Added lines #L377 - L378 were not covered by tests
[warning] 381-382: src/context.ts#L381-L382
Added lines #L381 - L382 were not covered by tests
[warning] 445-446: src/context.ts#L445-L446
Added lines #L445 - L446 were not covered by tests
[warning] 461-462: src/context.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
[warning] 465-466: src/context.ts#L465-L466
Added lines #L465 - L466 were not covered by tests
[warning] 469-470: src/context.ts#L469-L470
Added lines #L469 - L470 were not covered by tests
[warning] 473-474: src/context.ts#L473-L474
Added lines #L473 - L474 were not covered by tests
[warning] 477-478: src/context.ts#L477-L478
Added lines #L477 - L478 were not covered by tests
🔇 Additional comments (33)
src/application.ts (7)
14-14: Adoption of newContextimport
The explicit import ofContextsuggests a shift to more direct references instead of delegations. No issues found, but ensure that all dependent modules are consistently updated to avoid circular dependencies.
24-24: RefinedMiddlewareFunctemplate
DefiningMiddlewareFuncwith a default generic bound toContextincreases flexibility for custom contexts. This is a safe and clean approach to typed middleware.
30-30: GenericApplicationclass
AllowingApplicationto be parameterized by aContextsubtype is a good design for extending or customizing the context. This change should maintain backward compatibility while enabling richer types going forward.
44-45: Typedmiddlewarearray and async local storage
By providing a dedicated type to themiddlewarearray and usingAsyncLocalStorage<T>, concurrency data is properly associated with each request. However, keep an eye on memory usage under high load to avoid potential leaks.
47-51: Configurable context, request, and response classes
These prototypical class references afford dynamic subclassing. If you plan to override behavior, verify that these do not inadvertently mask important upstream constructor parameters.
87-91: Dynamic subclass creation
The dynamically extended classes (ApplicationContext,ApplicationRequest,ApplicationResponse) can improve maintainability for complex features. Verify coverage for all introduced methods, especially where custom overrides could occur.
199-199: Improved type safety in request handling
The updated middleware signature withctx: Tensures strong typing in your request pipeline. No immediate concerns found; just confirm that all error paths have test coverage.src/context.ts (4)
3-3: ImportingParsedUrlQuery
UsingParsedUrlQueryfosters strong typing forrequest.query, which can mitigate runtime-typing mistakes.
8-8: ImportingAcceptstype
This addition helps ensure robust type checking when retrieving accepted encodings, charsets, etc. Keep usage consistent across the file to avoid confusion.
227-383: Request delegation removal in favor of explicit getters and methods
All the new request delegation methods (e.g.,acceptsLanguages,acceptsEncodings, getters likequerystring,method, etc.) are clear and explicit. This improves readability over the previous delegate approach.However, static analysis shows lines (e.g., 273-274, 337-338, 341-342, etc.) lack test coverage. Consider adding tests to confirm baseline functionality of these new getters.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 273-274: src/context.ts#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 337-338: src/context.ts#L337-L338
Added lines #L337 - L338 were not covered by tests
[warning] 341-342: src/context.ts#L341-L342
Added lines #L341 - L342 were not covered by tests
[warning] 345-346: src/context.ts#L345-L346
Added lines #L345 - L346 were not covered by tests
[warning] 349-350: src/context.ts#L349-L350
Added lines #L349 - L350 were not covered by tests
[warning] 353-354: src/context.ts#L353-L354
Added lines #L353 - L354 were not covered by tests
[warning] 365-366: src/context.ts#L365-L366
Added lines #L365 - L366 were not covered by tests
[warning] 377-378: src/context.ts#L377-L378
Added lines #L377 - L378 were not covered by tests
[warning] 381-382: src/context.ts#L381-L382
Added lines #L381 - L382 were not covered by tests
384-483: Response delegation removal in favor of explicit getters and methods
Direct calls such asredirect(),has(),set(), and property access forstatus,body, etc., enhance overall clarity by making theContextAPI more explicit.Several lines (e.g., 445-446, 461-462, 465-466, etc.) lack coverage per the static analysis hints. Consider adding tests to validate basic operation (e.g., reading
length, updatingetag).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 445-446: src/context.ts#L445-L446
Added lines #L445 - L446 were not covered by tests
[warning] 461-462: src/context.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
[warning] 465-466: src/context.ts#L465-L466
Added lines #L465 - L466 were not covered by tests
[warning] 469-470: src/context.ts#L469-L470
Added lines #L469 - L470 were not covered by tests
[warning] 473-474: src/context.ts#L473-L474
Added lines #L473 - L474 were not covered by tests
[warning] 477-478: src/context.ts#L477-L478
Added lines #L477 - L478 were not covered by teststest/application.test-d.ts (1)
2-2: Aligned type references withContext
Switching the type fromContextDelegationtoContextkeeps the tests consistent with the code changes. Confirm that no usage scenarios remain that still rely on delegation-based logic.Also applies to: 5-5, 11-11
example/extend/middleware.ts (2)
1-1: Removed reference toContextDelegation
By importingContextdirectly, we simplify the type references and remove the transitional delegation code. Maintain updated imports throughout any extended classes.
10-10: Middleware uses a custom context
These typed definitions make it clear which custom properties and methods are available (ctx.hello). Confirm test coverage for your extended context usage.test/test-helpers/context.ts (1)
25-25: Directly returningapp.createContext
Eliminating the cast toanyorContextDelegationis safer, especially when dealing with generics. Confirm all test usage is updated to expect the newContexttype.test/application/context.test.ts (1)
3-3: Good import statement for type safety.
This import ensures that your subclass extends the correctContexttype.src/response.ts (6)
16-16: Context import for better type clarity.
ReplacingContextDelegationwith the directContextimport aligns with the removal of delegates.
19-19: Generic parameter ensures flexible context usage.
Declaringexport class Response<T extends Context = Context>properly integrates the custom context types.
24-24:ctx: Tproperty type strongly typed.
UsingTallows theResponseto access specialized context properties.
27-27: Constructor signature updated to accept genericctx.
This change ensuresResponseis constructed with the correct context subtype.
171-188: Refinedlengthgetter logic and types.
The explicitnumber | undefinedreturn type and checks covering string, buffer, and JSON content make its usage clearer and robust. This also prevents potential runtime errors from unaccounted content types.
281-281: Explicit return type fortypegetter.
Returning astringfor the MIME type is consistent with the broader type-safety improvements in this PR.src/request.ts (6)
12-12: DirectContextimport ensures consistency.
Removing theContextDelegationreference clarifies the class’s direct dependency onContext.
19-19: GenericRequestclass for advanced context customization.
This helps align the request-handling logic with custom context types.
24-24: Strong typing forctx: T.
Allows referencing specialized properties onctxwithinRequestwithout losing type information.
28-28: Updated constructor to accept a generic context.
Ensures theRequestinstance associates with the correct type of context.
145-145: Type-safe getter forquery.
ReturningParsedUrlQueryimproves confidence when accessing query properties.
161-161: Setter forqueryensures URL rewriting remains coherent.
Converting an object into a querystring is now strictly typed, mitigating potential runtime type errors.package.json (4)
12-16: Enhanced scripts for CI and test processes.
Usingpretest,preci, andpostciclarifies the workflow around linting, coverage, and packaging.
58-58:@eggjs/bindependency added.
This presumably aligns with your new build/CI approach.Would you like to verify its usage across scripts or confirm backward compatibility?
72-72: Updated@types/nodeto version"22".
Ensure that the Node.js typing changes do not break compatibility with older Node.js versions.
81-81: Bumpedmmto^4.0.1.
Check any major version changes or breaking differences inmmusage, especially around mocking behaviors.docs/api/response.md (1)
Line range hint
216-222: Verify implementation matches documentation.Let's verify that the documented behavior matches the actual implementation.
✅ Verification successful
Documentation matches implementation and tests
The documentation accurately reflects the implementation and behavior of the
appendmethod:
- The implementation in
src/response.tsshows that the method accepts a field (string) and value (string or string array) as parameters, which matches the documented signature.- The tests in
test/response/append.test.tsdemonstrate the documented behavior:
- Multiple values for the same header are properly handled (e.g.,
x-foo)- The example in the documentation using
Linkheader is actually tested in the test file- Both single values and arrays are supported as shown in the tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the append method implementation matches the documentation # Search for the append method implementation echo "Searching for append method implementation..." ast-grep --pattern 'append(field: string, val: string | string[]) { $$$ }' # Search for tests related to append echo "Searching for related tests..." rg -A 5 'test.*append.*header'Length of output: 283
Script:
#!/bin/bash # Let's try a different approach to find the append implementation and its tests # Search for files containing 'append' method echo "Searching for files with append method..." rg -l "append.*header|append.*field" # Search for the implementation with more context echo "Searching for append implementation..." rg -A 5 "append.*header|append.*field" # Search for test files containing 'append' echo "Looking for test files..." fd "test.*\.(js|ts|jsx|tsx)$" | xargs rg "append"Length of output: 2154
[skip ci] ## [2.20.3](v2.20.2...v2.20.3) (2025-01-02) ### Bug Fixes * remove delegates from Context ([#15](#15)) ([a3bdefe](a3bdefe))
Summary by CodeRabbit
Release Notes
Documentation
Responseobject, adding method to append headersDependencies
delegatesdependency@eggjs/bin@types/nodeandmmdependency versionsType Improvements
Application,Context,Request, andResponseclassesCode Refactoring
ContextclassTesting