@tus/server: use srvx for req/res conversion #749
Conversation
🦋 Changeset detectedLatest commit: 80c031a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as resolved.
This comment was marked as resolved.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub ↗.
|
![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 (1)
packages/server/src/server.ts (1)
136-147: Temporary workaround for header retrieval.This implementation provides a necessary compatibility layer for header retrieval while waiting for the issue to be fixed in srvx (referenced by issue #44). The implementation correctly handles edge cases like empty string values.
Consider adding a TODO comment with a link to the issue for easier tracking of when this workaround can be removed.
- // @ts-expect-error temporary until https://github.com/unjs/srvx/issues/44 is fixed + // TODO: Remove this workaround once https://github.com/unjs/srvx/issues/44 is fixed + // @ts-expect-error temporary workaround for header retrieval
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)packages/server/package.json(1 hunks)packages/server/src/server.ts(4 hunks)packages/server/src/test/Server.test.ts(2 hunks)packages/server/src/web.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/server/src/web.ts
🔇 Additional comments (11)
package.json (1)
4-4: Good addition of ESM module type.Adding
"type": "module"to the root package.json correctly enables ECMAScript modules for the entire project, which aligns with modern JavaScript practices and supports the integration with srvx.packages/server/package.json (2)
27-28: Appropriate addition of srvx dependency.The addition of the
srvxpackage as a dependency aligns with the PR objective to migrate the Node handler to use srvx for request/response conversion. The version pinning with caret allows for minor updates while maintaining compatibility.
35-35: Organizational improvement for dependency.Reordering the
@types/set-cookie-parsertype definition maintains alphabetical ordering in the devDependencies section, which is a good practice for readability.packages/server/src/test/Server.test.ts (4)
267-277: Improved test approach using supertest.Great refactoring of the HTTP origin test to use
supertestinstead of mocked HTTP objects. This provides a more realistic test scenario by making actual HTTP requests to the server.
280-320: Well-structured test cases for allowed origins.The test cases for HTTP origin handling have been refactored to use a consistent approach with
supertest, which improves readability and maintainability. The assertions properly verify that theAccess-Control-Allow-Originheader is set correctly based on the configured allowed origins.
322-332: Complete coverage for origin header scenarios.Good addition of a test case for when no origin header is present, ensuring that the fallback behavior works correctly by returning the first allowed origin from the configuration.
426-456: Excellent addition of test for custom request properties.This new test case verifies a key feature of the PR - ensuring that custom properties added to the request object (like in Express middleware) are preserved and accessible via
req.node.req. This directly supports the PR objective mentioned in the summary.packages/server/src/server.ts (4)
4-4: Good import of srvx handler.The import of
toNodeHandlerfrom 'srvx/node' is essential for implementing the req/res conversion, which is the core objective of this PR.
121-121: Simplified path registration.The refactored
getmethod now directly registers the path with the handler without appending the server's base path, making the path registration process cleaner and more maintainable.
125-125: Effective use of srvx for request handling.The
handlemethod has been streamlined by leveragingtoNodeHandlerfrom srvx, which simplifies the code and should provide better performance as mentioned in the PR objectives.
171-171: Explicit function call for context.abort().Explicitly calling
context.abort()as a function ensures the abort logic is executed correctly, addressing a potential bug where it might have been treated as a property access.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/green-ducks-return.md (1)
5-5: Fix grammatical issues in the description text.
Update the text to improve clarity and correct typos. Replace "for convert" with "to convert" and correct "orignal" to "original." For example, consider updating the line as follows:-Use srvx for convert Node.js req/res to Request/Response. This also comes with a performance boost. When using `server.handle()` in a Node.js environment, you can now access the orignal req/res via `req.node.req`/`req.node.res`. +Use srvx to convert Node.js req/res to Request/Response. This also comes with a performance boost. When using `server.handle()` in a Node.js environment, you can now access the original req/res via `req.node.req`/`req.node.res`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: --- "@tus/server": minor --- Use srvx for convert Node.js req/res to Request/Resp...(AI_HYDRA_LEO_REPLACE_FOR_TO)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/green-ducks-return.md(1 hunks)packages/server/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/server/README.md
🧰 Additional context used
🪛 LanguageTool
.changeset/green-ducks-return.md
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: --- "@tus/server": minor --- Use srvx for convert Node.js req/res to Request/Resp...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
🔇 Additional comments (1)
.changeset/green-ducks-return.md (1)
1-4: Changelog header and metadata formatting are correct.
The front matter (lines 1-4) is properly formatted for a changeset file. Please ensure that this formatting remains consistent across similar files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/server.ts (1)
137-148: Temporary workaround for srvx header retrievalThis is a reasonable temporary workaround for the srvx issue #44, but the implementation could be more efficient.
Consider using a more efficient approach that doesn't need to iterate through all headers for each lookup:
// @ts-expect-error temporary until https://github.com/unjs/srvx/issues/44 is fixed req.headers.get = (key: string) => { - for (const [k, v] of req.headers.entries()) { - if (k === key) { - if (v === '') { - return null - } - return v - } - } - return null + const value = [...req.headers.entries()].find(([k]) => k === key)?.[1]; + return value === '' ? null : value ?? null; }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/server.ts(5 hunks)packages/server/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/server/src/types.ts
🔇 Additional comments (5)
packages/server/src/server.ts (5)
4-5: Appropriate imports for srvx integrationThe new imports for
ServerRequesttype andtoNodeHandlerfunction are correctly added to support the migration to the srvx library for request/response conversion.
29-34: Type updates for event handlersGood update to the
TusEventsinterface to use theServerRequesttype from srvx instead of the previous request type, ensuring event handlers receive the correct request object type.
122-122: Simplified path registrationThe path registration has been simplified by registering paths directly without appending the server's base path.
126-126: Streamlined request handling with srvxUsing
toNodeHandlerefficiently converts Node.js HTTP request and response objects to the format expected by the handler method, which aligns with the migration goal of leveraging srvx for improved performance.
172-172: Bug fix in handleWeb methodCorrectly modified to explicitly call the
abortfunction, ensuring that the abort logic is executed properly.
1 participant
Closes #748
Ref: h3js/srvx#44
req.node.req