Feat fastify multer support #3019
Conversation
WalkthroughThis update enhances the framework's support for multer by introducing platform-specific imports and additional Fastify integration. Unused imports have been removed from the Express and Fastify components, and test coverage requirements have been raised. Integration tests and mock server configurations have been added for Express, Fastify, and Koa adapters. Relevant configuration files, including package.json and tsconfig mappings, were updated to support the new Fastify adapter and reorganized module paths. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as Fastify Server
participant MM as MulterModule (Fastify Plugin)
participant M as Multer Middleware
C->>F: Send multipart/form-data request
F->>MM: Call fastifyMulter (register parser & process request)
MM->>M: Create Multer instance with options
M->>F: Parse multipart data, attach files/body
F->>C: Send response with parsed data
Possibly related PRs
Suggested labels
✨ Finishing Touches
🪧 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: 25
🔭 Outside diff range comments (2)
packages/platform/platform-response-filter/src/services/__mock__/response.txt (1)
1-2: 🧹 Nitpick (assertive)New Mock File: Verify Placeholder Content
The file adds a simple placeholder with the word "test." Please ensure that this minimal content meets the mock requirements for your tests. If this file is meant to simulate a realistic response or structure, consider updating it with more descriptive or representative content.
lerna.json (1)
20-21: 🧹 Nitpick (assertive)Add newline at end of file
The file is missing a newline at the end, which is considered a best practice for text files.
-} \ No newline at end of file +} +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (46)
docs/docs/snippets/multer/configuration.ts(1 hunks)lerna.json(1 hunks)packages/platform/platform-express/src/components/PlatformExpress.ts(0 hunks)packages/platform/platform-fastify/.npmignore(1 hunks)packages/platform/platform-fastify/.swcrc(1 hunks)packages/platform/platform-fastify/package.json(1 hunks)packages/platform/platform-fastify/readme.md(1 hunks)packages/platform/platform-fastify/src/components/PlatformFastify.spec.ts(1 hunks)packages/platform/platform-fastify/src/components/PlatformFastify.ts(1 hunks)packages/platform/platform-fastify/src/index.ts(1 hunks)packages/platform/platform-fastify/src/interfaces/PlatformFastifySettings.ts(1 hunks)packages/platform/platform-fastify/src/interfaces/interfaces.ts(1 hunks)packages/platform/platform-fastify/src/services/PlatformFastifyRequest.spec.ts(1 hunks)packages/platform/platform-fastify/src/services/PlatformFastifyRequest.ts(1 hunks)packages/platform/platform-fastify/src/services/PlatformFastifyResponse.spec.ts(1 hunks)packages/platform/platform-fastify/src/services/PlatformFastifyResponse.ts(1 hunks)packages/platform/platform-fastify/test/app/Server.ts(1 hunks)packages/platform/platform-fastify/test/app/index.ts(1 hunks)packages/platform/platform-fastify/test/app/public/index.html(1 hunks)packages/platform/platform-fastify/test/app/views/view.ejs(1 hunks)packages/platform/platform-fastify/test/health.spec.ts(1 hunks)packages/platform/platform-fastify/test/platform-fastify.spec.ts(1 hunks)packages/platform/platform-fastify/test/test-session.integration.spec.ts(1 hunks)packages/platform/platform-fastify/tsconfig.esm.json(1 hunks)packages/platform/platform-fastify/vitest.config.mts(1 hunks)packages/platform/platform-http/src/common/services/PlatformHandler.ts(1 hunks)packages/platform/platform-multer/package.json(1 hunks)packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/express/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/MulterModule.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/src/adapters/koa/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/koa/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/vitest.config.mts(1 hunks)packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts(3 hunks)packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts(1 hunks)packages/platform/platform-response-filter/src/services/__mock__/response.txt(1 hunks)packages/platform/platform-router/src/domain/PlatformHandlerMetadata.spec.ts(6 hunks)packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts(1 hunks)packages/platform/platform-test-sdk/src/tests/testView.ts(1 hunks)packages/specs/scalar/src/middlewares/indexMiddleware.ts(2 hunks)packages/specs/swagger/src/middlewares/indexMiddleware.ts(2 hunks)tools/vitest/presets/index.js(1 hunks)tsconfig.node.json(1 hunks)tsconfig.spec.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/platform/platform-express/src/components/PlatformExpress.ts
🧰 Additional context used
🧠 Learnings (1)
packages/platform/platform-fastify/tsconfig.esm.json (1)
Learnt from: Romakita
PR: tsedio/tsed#2925
File: packages/platform/platform-fastify/tsconfig.esm.json:3-11
Timestamp: 2025-03-22T08:52:15.389Z
Learning: Dans le projet Ts.ED, lorsqu'un fichier `tsconfig` comme `packages/platform/platform-fastify/tsconfig.esm.json` étend `"@tsed/typescript/tsconfig.node.json"`, il n'est pas nécessaire de répéter les options du compilateur telles que `"moduleResolution": "node"` et `"sourceMap": true`, car elles sont déjà incluses dans le fichier de configuration étendu.
🧬 Code Definitions (10)
packages/platform/platform-http/src/common/services/PlatformHandler.ts (1)
packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts (1)
handler(64-66)
packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts (1)
packages/platform/platform-response-filter/src/constants/ANY_CONTENT_TYPE.ts (1)
ANY_CONTENT_TYPE(1-1)
packages/platform/platform-fastify/test/app/Server.ts (1)
packages/platform/platform-fastify/src/interfaces/interfaces.ts (1)
Configuration(18-24)
packages/platform/platform-fastify/src/components/PlatformFastify.spec.ts (1)
packages/platform/platform-fastify/src/components/PlatformFastify.ts (1)
PlatformFastify(39-332)
packages/platform/platform-fastify/src/services/PlatformFastifyResponse.spec.ts (1)
packages/platform/platform-fastify/src/services/PlatformFastifyResponse.ts (1)
PlatformFastifyResponse(19-100)
packages/platform/platform-fastify/src/interfaces/interfaces.ts (2)
packages/platform/platform-fastify/test/app/Server.ts (1)
Configuration(12-47)packages/platform/platform-fastify/src/interfaces/PlatformFastifySettings.ts (1)
PlatformFastifySettings(3-5)
packages/platform/platform-fastify/src/services/PlatformFastifyRequest.spec.ts (1)
packages/platform/platform-fastify/src/services/PlatformFastifyRequest.ts (1)
PlatformFastifyRequest(30-61)
packages/platform/platform-fastify/test/health.spec.ts (2)
packages/platform/platform-fastify/src/components/PlatformFastify.ts (1)
PlatformFastify(39-332)packages/platform/platform-fastify/test/test-session.integration.spec.ts (1)
Controller(36-74)
packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts (1)
packages/platform/platform-express/src/components/PlatformExpress.ts (1)
PlatformExpress(58-239)
packages/platform/platform-fastify/src/components/PlatformFastify.ts (4)
packages/platform/platform-fastify/src/interfaces/interfaces.ts (2)
Configuration(18-24)PlatformFastifyPluginSettings(14-14)packages/platform/platform-fastify/src/services/PlatformFastifyRequest.ts (3)
IncomingMessage(15-17)FastifyRequest(9-11)PlatformFastifyRequest(30-61)packages/platform/platform-fastify/src/interfaces/PlatformFastifySettings.ts (1)
PlatformFastifySettings(3-5)packages/platform/platform-fastify/src/services/PlatformFastifyResponse.ts (1)
PlatformFastifyResponse(19-100)
🪛 LanguageTool
packages/platform/platform-fastify/readme.md
[style] ~49-~49: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 2467 characters long)
Context: ...## Backers Thank you to all our backers! 🙏 [[Become a backer](https://opencolle...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.17.2)
packages/platform/platform-fastify/readme.md
1-1: Inline HTML
Element: p
(MD033, no-inline-html)
1-1: First line in a file should be a top-level heading
null
(MD041, first-line-heading, first-line-h1)
2-2: Inline HTML
Element: a
(MD033, no-inline-html)
2-2: Inline HTML
Element: img
(MD033, no-inline-html)
5-5: Inline HTML
Element: div
(MD033, no-inline-html)
6-6: Inline HTML
Element: h1
(MD033, no-inline-html)
19-19: Inline HTML
Element: div
(MD033, no-inline-html)
20-20: Inline HTML
Element: a
(MD033, no-inline-html)
21-21: Inline HTML
Element: span
(MD033, no-inline-html)
22-22: Inline HTML
Element: a
(MD033, no-inline-html)
23-23: Inline HTML
Element: span
(MD033, no-inline-html)
24-24: Inline HTML
Element: a
(MD033, no-inline-html)
25-25: Inline HTML
Element: span
(MD033, no-inline-html)
26-26: Inline HTML
Element: a
(MD033, no-inline-html)
29-29: Inline HTML
Element: hr
(MD033, no-inline-html)
45-45: Inline HTML
Element: a
(MD033, no-inline-html)
45-45: Inline HTML
Element: img
(MD033, no-inline-html)
45-45: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
51-51: Inline HTML
Element: a
(MD033, no-inline-html)
51-51: Inline HTML
Element: img
(MD033, no-inline-html)
51-51: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
🪛 Biome (1.9.4)
packages/platform/platform-multer/src/adapters/fastify/MulterModule.ts
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
packages/platform/platform-fastify/test/test-session.integration.spec.ts
[error] 67-67: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 15-29: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 30-34: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 35-74: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/platform/platform-fastify/src/components/PlatformFastify.ts
[error] 162-162: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 311-311: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-platform (20.12.2)
🔇 Additional comments (115)
packages/platform/platform-fastify/.npmignore (1)
1-7: Appropriate npm exclusion patternsThe .npmignore file correctly includes standard patterns for excluding test files, coverage reports, TypeScript configuration, mock directories, test specs, and build artifacts. This follows best practices for Node.js packages and will keep the published package clean and focused on production code.
packages/platform/platform-fastify/test/app/public/index.html (1)
1-5: Valid HTML test file structureThis basic HTML file is correctly structured and appears to be intended for testing the static file serving functionality of the Fastify platform integration.
packages/platform/platform-multer/vitest.config.mts (1)
21-21: Good practice: Added trailing newlineThe addition of a newline character at the end of the file follows good file formatting practices and avoids potential git diff issues.
packages/platform/platform-fastify/test/app/views/view.ejs (1)
1-1: Valid EJS template syntaxThe template correctly uses EJS syntax to dynamically render the variables
world,opts, andlocalIDwithin an HTML paragraph element. This template appears to be well-structured for testing view rendering with the Fastify platform integration.packages/platform/platform-test-sdk/src/tests/testView.ts (1)
61-61: Improved test flexibility for cross-platform compatibilityThe assertion has been updated to be more accommodating of different content-type header formats across platforms. By checking that the header contains "text/html" instead of requiring an exact match, this test will work more reliably across Express, Koa, and now Fastify platforms, which may format their content-type headers slightly differently.
docs/docs/snippets/multer/configuration.ts (1)
3-3: Import statement now includes Fastify as an optionThe configuration snippet has been updated to mention that multer platform support can now be imported from Express, Koa, or Fastify adapters. This enhancement aligns with the PR objective of adding Fastify multer support.
tools/vitest/presets/index.js (1)
12-12: Correctly added Fastify alias for multer platformThe alias for the Fastify adapter has been properly added to the Vitest configuration, following the same pattern as the existing Express and Koa adapters. This enables proper imports for
@tsed/platform-multer/fastifyduring testing.packages/platform/platform-fastify/.swcrc (1)
1-21: Well-structured SWC configuration for TypeScript and decoratorsThe SWC configuration is correctly set up for a TypeScript project that uses decorators, with appropriate settings for ES2022 target and module format. This configuration properly supports the TypeScript features required for the Fastify integration.
lerna.json (1)
17-17: Style improvement: Simplified array formatThe array formatting has been changed from multi-line to single-line, which is a reasonable style change for a small array.
packages/platform/platform-fastify/src/interfaces/PlatformFastifySettings.ts (1)
1-5: Well-structured interface for Fastify platform settingsThis new interface follows the pattern used by other platform adapters in Ts.ED, allowing users to provide an existing Fastify instance if needed. The typing is correct and includes proper type imports from the Fastify package.
packages/platform/platform-http/src/common/services/PlatformHandler.ts (1)
43-43:✅ Verification successful
Storing the compiled handler for future reference
This change stores the processed handler (either the injectable handler or the original) in the
compiledHandlerproperty of the handler metadata. This allows other parts of the code to access the finalized handler directly.
🏁 Script executed:
#!/bin/bash # Check if compiledHandler is properly defined in PlatformHandlerMetadata class echo "Checking if compiledHandler property is properly defined in the PlatformHandlerMetadata class:" rg -A 3 -B 3 "compiledHandler" --type ts packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts # Check where compiledHandler is used in the codebase echo "Checking usages of compiledHandler in the codebase:" rg "compiledHandler" --type tsLength of output: 1336
Validated: Proper use of the compiledHandler property
The change correctly stores the processed handler into
handlerMetadata.compiledHandler, ensuring that the finalized handler is easily accessible by other components. Verification details:
- Declaration Check: The
compiledHandlerproperty is defined ascompiledHandler: ($ctx: DIContext) => any;in thePlatformHandlerMetadataclass.- Usage Confirmation: This property is consistently referenced in the codebase, including in
PlatformFastifyand found in other modules like theAlterActionscomponent.Overall, this update aligns with the intended design, so no further changes are needed.
packages/specs/swagger/src/middlewares/indexMiddleware.ts (2)
17-17: Improved response handling with explicit content typeGood improvement to explicitly set the content type to "text/html" before sending the response body. This ensures the browser correctly interprets the HTML content and follows best practices for HTTP response handling.
1-1: Clean import consolidationGood refactoring to consolidate the imports from node:path into a single line, improving code readability.
packages/platform/platform-fastify/test/app/Server.ts (2)
1-48: Well-structured Fastify test server configurationThis test server configuration provides a comprehensive setup for testing the Fastify adapter with all necessary components:
- Proper imports for required features
- Complete server configuration with port, static files, and views
- Well-organized plugin configuration
The TypeScript
satisfieskeyword on line 35 is a good use of type checking to ensure session options match the expected interface.
37-43: Raw body configurationThe configuration for
fastify-raw-bodywithglobal: false, runFirst: trueis appropriate for selective raw body processing while ensuring it runs before other parsers. This is important for proper multipart/form-data handling with Multer.packages/specs/scalar/src/middlewares/indexMiddleware.ts (2)
19-19: Consistent improvement in response handlingGood improvement in explicitly setting the content type to "text/html", consistent with the changes in the swagger middleware. This standardized approach ensures proper content type interpretation across the framework.
1-1: Clean import consolidationGood refactoring to consolidate the imports from node:path into a single line, maintaining consistency with other files.
packages/platform/platform-multer/src/adapters/express/__mock__/Server.ts (3)
1-17: Well-structured Express mock server configurationGood implementation of a mock Express server configuration that provides the necessary setup for testing the Multer Express adapter. The configuration includes all required middleware components and follows the project's conventions.
8-8: Informative comment about build-time replacementGood practice to include the comment explaining that
import.meta.dirnamewill be automatically replaced during the build process. This helps other developers understand the build-time transformations.
15-15: Comprehensive middleware configurationThe middleware configuration is well-organized and includes all necessary components for an Express server. The use of both string references and object configurations demonstrates flexibility in the framework's middleware system.
packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts (1)
142-149: Well-implemented middleware type checker!The
isRawMiddleware()method is correctly implemented to identify non-injectable handlers of typeRAW_FNorRAW_ERR_FN. The JSDoc comment clearly explains the purpose of this method, which will be useful for properly identifying and handling raw middleware functions in the Fastify-Multer integration.packages/platform/platform-router/src/domain/PlatformHandlerMetadata.spec.ts (6)
37-37: Good test coverage for method with context handler!This correctly tests that the
isRawMiddleware()method returnsfalsefor context handlers, which aligns with the implementation.
54-54: Appropriate test for raw functions!This correctly verifies that the
isRawMiddleware()method returnstruefor standard raw middleware functions with the next parameter.
72-72: Proper test for error handling middleware!This correctly verifies that the
isRawMiddleware()method returnstruefor raw error middleware functions.
91-91: Good test for functions without next parameter!This correctly verifies that the
isRawMiddleware()method returnstruefor raw middleware functions without the next parameter.
124-124: Complete test coverage for injected endpoints!This correctly verifies that the
isRawMiddleware()method returnsfalsefor endpoint handlers with dependency injection.
152-152: Comprehensive test for injected error middleware!This correctly verifies that the
isRawMiddleware()method returnsfalsefor error middleware with dependency injection.packages/platform/platform-multer/src/adapters/fastify/__mock__/Server.ts (2)
6-9: Import path management seems correct.The configuration and rootDir setup is appropriate for the testing environment.
10-27: Well-structured Fastify configuration.The server configuration includes appropriate plugins for a Fastify server that will work with file uploads:
@fastify/acceptsfor content negotiation@fastify/cookiefor cookie handling@fastify/formbodyfor parsing form bodiesfastify-raw-bodyconfigured to not be global and run firstThis configuration prepares the groundwork for Multer integration.
packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.ts (2)
1-5: Appropriate imports for testing the Fastify adapter.The necessary dependencies are imported for testing Fastify with Multer.
15-19:✅ Verification successful
Minimal but sufficient test structure.
The test structure is minimal but appears to rely on a pre-configured test utility. This approach is fine, but consider adding more specific assertions if there are particular aspects of the Multer integration that need validation.
To better understand what the
utils.test("multer")method is testing, let's check if there's a test definition file:
🏁 Script executed:
#!/bin/bash # Search for test definition files related to multer fd --type file multer packages/platform/platform-test-sdk -x grep -l "multer" {} \;Length of output: 146
Test Structure Verification and Confirmation
The minimal test in
packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.tsdelegates its assertions to the pre-configured test utility viautils.test("multer"). Our search confirms that the detailed test definitions for Multer are located in:
packages/platform/platform-test-sdk/src/tests/testMulter.tsThis centralized approach is acceptable as long as the dedicated test file comprehensively covers the expected behaviors of Multer integration. If further granular assertions are needed at the integration level, consider adding them; otherwise, the current setup is sufficient.
packages/platform/platform-fastify/test/platform-fastify.spec.ts (4)
1-11: Setup looks good with clear adapter configuration.The test setup is well structured, correctly importing the necessary modules and configuring the PlatformTestSdk with the new PlatformFastify adapter.
12-28: Test structure is comprehensive with appropriate skipping.The test structure covers handlers, controllers, and response functionality. The skipped stream tests are properly commented as "untestable with Fastify", which is helpful for other developers.
29-82: Good coverage of core platform functionality.This section thoroughly tests various aspects of the platform including middlewares, request scopes, parameters handling, response filters, routing, etc. The skipped session test is also properly documented.
84-100: Plugin tests are present including the new Multer support.The test suite includes tests for various plugins, including the newly added Multer support which is the focus of this PR. The DeepQueryParams test is appropriately skipped.
packages/platform/platform-fastify/tsconfig.esm.json (2)
12-25: Exclude patterns look comprehensive.The exclusion patterns properly exclude test files, build artifacts, and other non-source directories. This ensures a clean build output.
1-11:❓ Verification inconclusive
Consider simplifying compiler options.
Based on previous feedback, some compiler options might be redundant since they're already defined in the extended configuration file.
🏁 Script executed:
#!/bin/bash # Check what options are already defined in the base tsconfig cat node_modules/@tsed/typescript/tsconfig.node.json | grep -A 20 "compilerOptions"Length of output: 164
Attention: Verify Extended tsconfig Configuration
The automated check was unable to locate the base configuration file
@tsed/typescript/tsconfig.node.jsonin the expected location (typically within yournode_modulesdirectory). Please ensure that your dependencies are installed so that this file becomes available. If the file does exist once dependencies are installed, verify whether itscompilerOptionsalready define settings such asbaseUrl,rootDir,outDir,declarationDir,declaration,composite, andnoEmit. If these options are indeed inherited from the base configuration, simplify the compiler options inpackages/platform/platform-fastify/tsconfig.esm.jsonaccordingly to avoid redundancy.packages/platform/platform-fastify/src/index.ts (1)
1-8: Export structure looks correct.The barrel file properly exports all necessary components from the Fastify platform adapter. The file header correctly indicates it was auto-generated.
packages/platform/platform-fastify/readme.md (2)
1-32: Header and navigation structure is well-organized.The header with logo, badges, and navigation links follows the project's style guide and provides users with essential information.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Inline HTML
Element: p(MD033, no-inline-html)
1-1: First line in a file should be a top-level heading
null(MD041, first-line-heading, first-line-h1)
2-2: Inline HTML
Element: a(MD033, no-inline-html)
2-2: Inline HTML
Element: img(MD033, no-inline-html)
5-5: Inline HTML
Element: div(MD033, no-inline-html)
6-6: Inline HTML
Element: h1(MD033, no-inline-html)
19-19: Inline HTML
Element: div(MD033, no-inline-html)
20-20: Inline HTML
Element: a(MD033, no-inline-html)
21-21: Inline HTML
Element: span(MD033, no-inline-html)
22-22: Inline HTML
Element: a(MD033, no-inline-html)
23-23: Inline HTML
Element: span(MD033, no-inline-html)
24-24: Inline HTML
Element: a(MD033, no-inline-html)
25-25: Inline HTML
Element: span(MD033, no-inline-html)
26-26: Inline HTML
Element: a(MD033, no-inline-html)
29-29: Inline HTML
Element: hr(MD033, no-inline-html)
33-39: Installation instructions are clear.The installation command correctly includes both the platform package and the required Fastify dependency.
packages/platform/platform-fastify/src/components/PlatformFastify.spec.ts (2)
7-20: LGTM - Well-structured test for the create() methodThe test is well-organized with proper setup and teardown, effectively verifying that the
createmethod correctly passes the adapter to the PlatformBuilder.
21-33: LGTM - Good async test implementation for bootstrapThe bootstrap test correctly uses async/await syntax and properly verifies the method parameters.
packages/platform/platform-multer/src/adapters/koa/__mock__/Server.ts (3)
9-10: LGTM - Good modern syntax usageThe use of
import.meta.dirnamefor module path resolution is a good practice for modern ESM modules.
29-56: Security settings in session configuration are insecure for productionWhile this is acceptable for a mock server used in testing, several security-related settings are set to
falsewhich would be problematic in a production environment:
httpOnly: falseallows client-side JavaScript to access cookiessigned: falsemeans cookies aren't cryptographically signedsecure: falseallows sending cookies over non-HTTPS connectionsSince this is a mock file for testing, these settings are likely intentional, but worth noting in case this configuration is ever copied for production use.
12-24: LGTM - Comprehensive server configurationThe configuration decorator provides a complete setup with all necessary properties for the server including port, static file paths, view settings, and middleware configuration.
packages/platform/platform-multer/package.json (1)
29-33: LGTM - Properly added Fastify adapter exportsThe Fastify adapter export entries follow the same pattern as the existing Express and Koa adapters, maintaining consistency in the package structure. This addition enables Fastify support for handling multipart form data.
packages/platform/platform-multer/src/adapters/koa/MulterModule.integration.spec.ts (1)
6-13: LGTM - Well-configured test utility setupThe test utility is properly configured with all necessary parameters including the root directory, adapter, server, and logging settings.
packages/platform/platform-fastify/test/app/index.ts (1)
10-21: Controller looks good.
Registering two GET routes with parameter handling is straightforward. The usage of decorators is correct, and the returned JSON structure aligns with typical Ts.ED patterns.packages/platform/platform-fastify/test/health.spec.ts (1)
40-49: Good integration test setup.
You’re verifying the endpoint’s behavior and ensuring a 200 response. The usage ofPlatformTest.callback()is proper for Ts.ED platform testing. The response structure matches your controller.packages/platform/platform-fastify/package.json (2)
18-25: Scripts are comprehensive.
The build and test scripts cover typical usage, and runningvitestfor coverage is good practice. No major issues spotted.
1-4:✅ Verification successful
Validate version alignment with Ts.ED ecosystem.
You’ve set"version": "8.7.1"which may need to align with other packages in the monorepo. Confirm it matches the intended release tag to avoid mismatched versions among related modules.
🏁 Script executed:
#!/bin/bash # Validate that package.json versions align with the monorepo's global versions rg '"version": "8.7.1"'Length of output: 4544
Version alignment confirmed: "8.7.1" is consistent across the Ts.ED monorepo.
The@tsed/platform-fastifypackage now correctly matches the global version used in all related modules, as verified by the grep results.packages/platform/platform-fastify/src/services/PlatformFastifyResponse.ts (4)
8-13: Extension of the global TsED.Response interface.
Defining thelocalsproperty directly ensures consistency across Ts.ED modules, but confirm that settingthis.raw.locals = {}in the constructor is safe per-request and not shared by multiple requests.
42-45: Status method usage is correct.
Usingthis.raw.code(status)aligns well with Fastify's recommended approach. No concerns here.
78-81: Use of content-disposition for file download.
Great approach. Setting the header with a properly formatted value helps ensure correct file handling by clients.
89-99: JSON and stream methods.
Sending data directly withthis.raw.send(data);is straightforward and consistent with Fastify’s API. No issues identified.packages/platform/platform-fastify/src/services/PlatformFastifyRequest.spec.ts (5)
5-15: Well-structured test setup function.The
createRequesthelper function provides a clean way to initialize test objects, making the tests more readable and maintainable.
17-25: LGTM: Basic test functionality looks good.The test correctly verifies that the PlatformFastifyRequest instance is created with the raw request object.
27-34: LGTM: Good test for secure property.The test correctly verifies the
secureproperty based on the protocol.
36-44: LGTM: Host property test looks good.The test correctly verifies that the
hostproperty returns the hostname from the request.
46-53: LGTM: GetReq method test is correct.The test verifies that
getReq()returns the raw Node.js request object.packages/platform/platform-fastify/src/services/PlatformFastifyResponse.spec.ts (7)
5-19: Good test setup function with Fastify specifics.The
createResponsehelper function properly sets up Fastify-specific method aliases (codeforstatus,typeforcontentType, etc.), which ensures tests are Fastify-compatible.
31-40: LGTM: GetRes method test looks good.The test correctly verifies that the
getRes()method returns the raw response object.
41-49: LGTM: StatusCode property test is correct.The test properly verifies the
statusCodeproperty after setting the status.
50-58: LGTM: ContentType method test is thorough.The test correctly verifies that the
contentType()method sets the content-type header.
59-67: LGTM: Body method test is correct.The test properly verifies that the
body()method sets the response body.
69-77: LGTM: Stream method test is correct.The test properly verifies that the
stream()method sets the response body.
78-113: LGTM: Location method tests are comprehensive.The tests thoroughly cover different scenarios for the
location()method:
- Setting a specific location
- Going back based on the Referrer header
- Falling back to a default path
packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts (4)
34-47: LGTM: Buffer content type test is appropriate.The test correctly verifies that
undefinedis returned for Buffer inputs, which is important for binary data handling.
64-77: LGTM: Default content type test for objects.The test correctly verifies that objects without a specific content type are resolved to "application/json", which is the expected behavior.
48-63:✅ Verification successful
Test for stream content type has a potential issue.
The test correctly verifies the behavior for stream inputs, but it uses
import.meta.dirnamewhich might not be supported in all environments. Consider using__dirnameor a more environment-agnostic approach.
🏁 Script executed:
#!/bin/bash # Checking if import.meta.dirname is used across the codebase or if there's a helper function # Look for import.meta.dirname usage rg -A 1 "import\.meta\.dirname" # Check if there's a common pattern for handling file paths in tests rg -A 1 "__mock__" --glob "*.spec.ts"Length of output: 23464
Stream content type test—usage of
import.meta.dirnameis valid
After verification, the usage ofimport.meta.dirnamein this test is consistent with established patterns across the codebase. Our build process automatically replaces and handles these references for ESM modules, so the potential environment issues raised do not apply here. No changes are needed.
3-3:✅ Verification successful
Update Node.js import to use the node: protocol.
The import from the Node.js builtin module 'fs' should use the node: protocol for clarity and to follow best practices.
-import {createReadStream} from "fs"; +import {createReadStream} from "node:fs";
🌐 Web query:
Why should Node.js builtin modules use the node: protocol in import statements?💡 Result:
Node.js introduced the
node:protocol for built-in modules to address several key issues and improve code clarity. Here are the primary reasons to use it:1. Prevent Module Overriding Conflicts
Using
node:explicitly identifies core modules, preventing accidental use of third-party packages with the same name innode_modules[1][4][7]. For example:// Without node: protocol (could be hijacked) import fs from 'fs'; // Explicit core module reference import fs from 'node:fs/promises';This ensures you're always using the official Node.js implementation [4][9].
2. Explicit Code Clarity
The protocol clearly distinguishes core modules from other dependencies:
// Obvious native module import crypto from 'node:crypto'; // Potentially ambiguous import express from 'express';This explicit syntax helps developers quickly identify built-in dependencies [1][7][9].
3. Tooling Support
Modern tooling leverages
node:for:
- TypeScript: Requires
@types/node@16+for proper type resolution [4][9]- ESLint:
eslint-plugin-importoffers rules to enforce protocol usage [3][7]- Bundlers: Explicit protocol helps with static analysis and tree-shaking [1][7]
4. Forward Compatibility
The protocol future-proofs your code against potential naming conflicts in Node.js's evolving ecosystem [1][4]. It's supported in:
- Node.js v14.18.0+ (ESM)
- Node.js v16.0.0+ (CommonJS and ESM)
- TypeScript 4.7+ with updated type definitions [4][7]
5. Ecosystem Best Practices
Major style guides and linting tools now recommend
node:protocol:// Recommended by: // - eslint-plugin-unicorn [7] // - Biome [9] // - Node.js documentation [11] import path from 'node:path';These tools can automatically fix missing protocols through their --fix options [3][7][9].
While optional, using
node:protocol has become a de facto standard for writing unambiguous, future-proof Node.js code. The main exception would be when maintaining legacy systems that predate protocol support [3][4].Citations:
- 1: https://2ality.com/2021/12/node-protocol-imports.html
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import
- 3: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/enforce-node-protocol-usage.md
- 4: https://dev.to/yenyih/new-nodejs-protocol-imports-3pnf
- 5: https://forum.moralis.io/t/solved-node-js-cannot-use-import-statement-outside-a-module/18826
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules
- 7: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-node-protocol.md
- 8: https://www.jonathancreamer.com/write-async-node-scripts-using-the-only-native-nodejs-modules/
- 9: https://biomejs.dev/linter/rules/use-nodejs-import-protocol/
- 10: https://www.w3schools.com/nodejs/ref_modules.asp
- 11: https://nodejs.org/api/esm.html
- 12: https://nodejs.org/api/modules.html
- 13: Use the
node:protocol in docs examples nodejs/node#38343
Update Node.js Import to Use Explicit
node:ProtocolThis change updates the import for the Node.js built-in module to explicitly use the
node:protocol. Using thenode:protocol prevents potential conflicts with third-party packages, improves code clarity, and aligns with modern tooling standards. Please update the import as shown:-import {createReadStream} from "fs"; +import {createReadStream} from "node:fs";🧰 Tools
🪛 Biome (1.9.4)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts (5)
1-1: LGTM: Good import for utility functions.Properly importing utility functions from
@tsed/coreto check object types and streams.
8-19: Improved type safety and function documentation.The function now uses
unknowninstead ofanyfor better type safety, and the JSDoc has been improved with more accurate parameter types.
20-20: LGTM: Simplified content type determination.The code now directly gets the content type from the response or operation, removing unnecessary fallback to an empty string.
23-23: LGTM: Simplified conditional logic.Concise handling of application/json content type when the data is an object.
29-30: LGTM: Improved object type detection.The function now explicitly checks if the data is an object, not a stream, and not a buffer before defaulting to "application/json", which improves handling of different data types.
packages/platform/platform-fastify/src/interfaces/interfaces.ts (1)
16-26: Configuration extension looks good.The global declaration correctly extends TsED’s Configuration interface with Fastify-specific settings and plugin definitions. This approach provides a well-typed setup, so no further concerns here.
packages/platform/platform-fastify/src/services/PlatformFastifyRequest.ts (1)
31-33:❓ Verification inconclusive
Confirm the availability of
hostnameon the raw request object.Fastify’s request object usually indicates the hostname via
raw.headers.hostrather thanraw.hostname. Ensure thatthis.raw.hostnameis reliably populated in all scenarios. If uncertain, consider falling back tothis.raw.headers.host.
🏁 Script executed:
#!/bin/bash # Description: Search for references to `this.raw.hostname` to confirm usage in the codebase and check if it’s properly supported. rg --context 5 "this\.raw\.hostname"Length of output: 1085
Action Required: Verify Implementation of Raw Request Hostname
The file
packages/platform/platform-fastify/src/services/PlatformFastifyRequest.tscurrently retrieves the host usingthis.raw.hostname. Given that Fastify's raw request object (an instance of Node’s IncomingMessage) typically does not include ahostnameproperty and that the hostname is usually derived fromraw.headers.host, please confirm thatthis.raw.hostnameis reliably populated in your deployment scenarios. Consider implementing a fallback such as:get host(): string { return this.raw.hostname || this.raw.headers.host; }
- Validate that the Fastify request object setup or any custom middleware is indeed setting
hostname.- If not guaranteed, ensure that the fallback to
this.raw.headers.hostis in place to avoid potential runtime issues.packages/platform/platform-fastify/test/test-session.integration.spec.ts (2)
1-15: No issues found with the imports.
They provide the necessary dependencies for session-related integration tests.
82-82: Remove or confirm usage ofdescribe.skip().
Currently, all tests in this suite are skipped. Confirm if this is intentional or remove.skipto ensure this test suite runs.tsconfig.node.json (12)
9-10: Reorganized path mappings for engines and perf.
No functional issues found.
22-22: New path mapping for Fastify Multer adapter.
This correctly points to the new Fastify Multer module.
37-39: Re-introduced path for Platform Fastify and GraphQL-WS.
The reorganization looks consistent with the rest of the Ts.ED packages.
48-48: Re-added Mongoose path.
No concerns; mapping aligns with the Mongoose adapter.
55-55: Re-added path for Serverless HTTP.
No issues; appears consistent with the new file structure.
59-59: Re-added path for OIDC wildcard redirect URI plugin.
No concerns; consistent with the reorganized module references.
62-62: New path for event emitter.
Looks properly targeted.
65-66: Re-added path for components-scan and bullmq.
This reorganization does not pose functional issues.
70-70: Re-added path for schema-formio.
No issues identified.
72-72: New path for SSE.
Properly mapped to the SSE package.
74-74: Re-added path for vike.
No concerns identified.
75-75: Continuing reorganization with temporal.
Mapping is consistent.tsconfig.spec.json (12)
12-13: Path mappings for engines and perf.
No issues found.
25-25: New path mapping for Fastify Multer adapter.
Consistent with the new Fastify support.
40-40: Re-introduced path for platform-fastify.
Looks aligned with the production tsconfig.
42-42: Re-added mapping for graphql-ws.
Checks out; no concerns.
51-51: Re-added mapping for mongoose.
No issues; consistent reorganization.
58-58: Re-introduced serverless-http path.
In line with the main tsconfig changes.
62-62: New mapping for OIDC wildcard redirect URI.
No issues.
65-65: New event emitter path.
Mapping is correct.
68-69: Re-added paths for components-scan and bullmq.
No further concerns.
73-73: Re-added path for schema-formio.
Matches the established pattern.
75-75: New path for SSE.
Looks good.
77-78: Re-added vike and introduced temporal.
No issues.packages/platform/platform-fastify/src/components/PlatformFastify.ts (11)
1-2: Multer Fastify integration import.
This import swiftly loads the new Fastify adaptation for Multer.
39-41: Class definition looks good.
PlatformFastifyappropriately extends the basePlatformAdapter.
67-101: Proper handling of routes inmapLayers().
Layers are correctly mapped to Fastify routes. Thetry/catchensures graceful fallback on route definition errors.
102-131: Well-structuredmapHandler()logic.
Middleware vs. route handlers are properly distinguished. The context-based approach withrunInContextfosters consistent lifecycle management.
132-164: Context creation and plugin registration inuseContext().
RegisteringfastifyMiddiefor Express-like middleware support is a good approach. The hook architecture ensures Ts.ED context is set up before request handling.🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
166-203:createApp()method properly configures Fastify.
HTTP and HTTPS options are merged correctly, and request/reply decorators are in place for the Ts.ED context.
205-222:afterLoadRoutes()sets custom error and not found handlers.
Clean usage ofPlatformExceptionsfor error-handling; ensures Ts.ED’s centralized exception logic is integrated.
224-252:getServers()cleanly configures both HTTP and HTTPS.
UsescreateServerwith a straightforwardlistencallback. No issues.
258-267: Static file serving withstatics().
Leverages@fastify/staticand togglesdecorateReplyonce. Straightforward approach.
269-296: Middleware composition incompose().
Executes multiple handlers in a context-aware manner, capturing errors for re-throwing at the end.
333-343: Adapter registration for request/response.
PlatformFastifyRequestandPlatformFastifyResponseare swapped in seamlessly, finalizing the integration.
packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts
Show resolved
Hide resolved
packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.ts
Show resolved
Hide resolved
packages/platform/platform-fastify/test/test-session.integration.spec.ts
Outdated
Show resolved
Hide resolved
packages/platform/platform-fastify/test/test-session.integration.spec.ts
Outdated
Show resolved
Hide resolved
packages/platform/platform-fastify/src/components/PlatformFastify.ts
Outdated
Show resolved
Hide resolved
packages/platform/platform-fastify/src/components/PlatformFastify.ts
Outdated
Show resolved
Hide resolved
packages/platform/platform-fastify/src/components/PlatformFastify.ts
Outdated
Show resolved
Hide resolved
Romakita
force-pushed
the
feat-fastify-multer-support
branch
from
77477aa to
9ae0f82
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
docs/docs/snippets/multer/configuration.ts(1 hunks)packages/platform/platform-express/src/components/PlatformExpress.ts(0 hunks)packages/platform/platform-fastify/package.json(3 hunks)packages/platform/platform-fastify/src/components/PlatformFastify.ts(3 hunks)packages/platform/platform-fastify/test/platform-fastify.spec.ts(1 hunks)packages/platform/platform-fastify/tsconfig.json(0 hunks)packages/platform/platform-fastify/tsconfig.spec.json(0 hunks)packages/platform/platform-fastify/vitest.config.mts(1 hunks)packages/platform/platform-multer/package.json(1 hunks)packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/express/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/MulterModule.ts(1 hunks)packages/platform/platform-multer/src/adapters/fastify/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/src/adapters/koa/MulterModule.integration.spec.ts(1 hunks)packages/platform/platform-multer/src/adapters/koa/__mock__/Server.ts(1 hunks)packages/platform/platform-multer/vitest.config.mts(1 hunks)packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts(1 hunks)tools/vitest/presets/index.js(1 hunks)tsconfig.node.json(1 hunks)tsconfig.spec.json(1 hunks)
💤 Files with no reviewable changes (3)
- packages/platform/platform-fastify/tsconfig.json
- packages/platform/platform-express/src/components/PlatformExpress.ts
- packages/platform/platform-fastify/tsconfig.spec.json
🧰 Additional context used
🧬 Code Definitions (3)
packages/platform/platform-multer/src/adapters/koa/__mock__/Server.ts (4)
packages/platform/platform-multer/src/adapters/express/__mock__/Server.ts (1)
rootDir(8-8)packages/platform/platform-multer/src/adapters/fastify/__mock__/Server.ts (1)
rootDir(8-8)packages/platform/platform-express/src/components/PlatformExpress.ts (1)
Application(43-43)packages/platform/platform-fastify/src/components/PlatformFastify.ts (1)
Application(34-34)
tools/vitest/presets/index.js (2)
tools/vitest/presets/alias.js (2)
alias(59-64)alias(59-64)packages/di/src/common/services/InjectorService.ts (1)
alias(119-123)
packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts (1)
packages/platform/platform-express/src/components/PlatformExpress.ts (1)
PlatformExpress(58-239)
🪛 Biome (1.9.4)
packages/platform/platform-multer/src/adapters/fastify/MulterModule.ts
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🔇 Additional comments (28)
tsconfig.node.json (1)
22-22: Great addition of Fastify multer support!This new path mapping for
@tsed/platform-multer/fastifycorrectly integrates the Fastify adapter into the TypeScript configuration, aligning with the PR objective of supporting Fastify with Multer.packages/platform/platform-multer/vitest.config.mts (1)
21-21: Trailing newline looks good.Adding a newline at the end of the file follows good coding practices and complies with most linting rules.
docs/docs/snippets/multer/configuration.ts (2)
3-3: Well-documented import options!The comment clearly indicates that users can choose between Express, Koa, or the newly added Fastify adapter depending on their platform preference. This is an excellent addition to the documentation.
1-11:❓ Verification inconclusive
Verify Fastify integration with complete configuration example
The existing example shows the basic configuration, but it would be helpful to verify if Fastify users need any additional configuration parameters.
🏁 Script executed:
#!/bin/bash # Look for existing Fastify-specific configuration examples or tests grep -r "multer.*fastify" --include="*.ts" --include="*.js" packages/ | grep -v "MulterModule.ts" # Check for any Fastify-specific tests find packages/ -name "*.spec.ts" -o -name "*.test.ts" | xargs grep -l "fastify.*multer"Length of output: 296
Fastify multer configuration: Verify if a complete example is required
The current snippet in
docs/docs/snippets/multer/configuration.tsshows a basic multer configuration (with an Express import) and includes a comment about alternatives (e.g.@tsed/platform-multer/fastify). However, our grep output only confirms that Fastify integration is imported (inpackages/platform/platform-fastify/src/components/PlatformFastify.ts) without revealing any Fastify-specific configuration parameters.Please verify whether Fastify users need additional configuration parameters compared to the basic setup. If a more comprehensive Fastify example is required, kindly update the snippet or the accompanying documentation to clarify the intended setup.
tools/vitest/presets/index.js (1)
12-12: Correctly configured Fastify multer alias for testingThe alias is properly structured to follow the same pattern as the existing Express and Koa aliases, ensuring consistent test resolution across all adapters.
packages/platform/platform-fastify/vitest.config.mts (2)
13-16: Great job updating the coverage thresholds to proper values!The coverage thresholds have been significantly increased from 0 to meaningful values (statements: 93.84%, branches: 91.42%, functions: 90%, lines: 93.84%). This is a positive change that enforces robust test coverage for the Fastify adapter.
1-1: 🧹 Nitpick (assertive)Remove the @ts-ignore comment
The
@ts-ignorecomment on line 1 should be avoided unless absolutely necessary. Consider fixing the typing issue or using a more specific@ts-expect-errorwith an explanation.packages/platform/platform-fastify/test/platform-fastify.spec.ts (1)
91-91: Great job enabling the Multer plugin tests!Removing the
.skipdirective enables the Multer plugin tests to be executed, which aligns with the PR objective of adding Fastify Multer support. This ensures that your new implementation is properly tested.packages/platform/platform-fastify/src/components/PlatformFastify.ts (4)
1-2: Good addition of the Multer plugin importThe import for
@tsed/platform-multer/fastifyis correctly added to enable Multer support in Fastify. This is in line with the PR objective of adding Fastify Multer support.
331-334: Great implementation of the $alter hook for plugin resolutionUsing the
$alterhook with$afterPluginsallows for dynamic modification of the plugin list, which facilitates the integration of the Multer plugin. This is a more flexible approach than the previous implementation.
162-162: 🧹 Nitpick (assertive)Avoid using the non-null assertion.
At line 162, this code uses
plugin.options!. Non-null assertions can mask potential runtime issues and are discouraged. Consider validatingplugin.optionsor providing a default instead.- await rawApp.register(plugin.use as any, plugin.options!); + await rawApp.register(plugin.use as any, plugin.options || {});
311-312: 🧹 Nitpick (assertive)
resolvePlugins()reassigns function parameter.At line 311, the code does
plugin = {env, use: plugin};which can be confusing. Consider creating a local variable instead of reassigning the argument.- if (isString(plugin)) { - plugin = {env, use: plugin}; - } + if (isString(plugin)) { + const pluginConfig = {env, use: plugin}; + return pluginConfig; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 312-312: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/platform/platform-router/src/domain/PlatformHandlerMetadata.ts (1)
147-147: Good type safety improvementAdding the explicit return type
: booleanto theisRawMiddleware()method improves type safety and code readability. This is a good practice, especially for public methods.packages/platform/platform-multer/src/adapters/express/MulterModule.integration.spec.ts (1)
1-19: Well-structured integration test for Multer with Express.The integration test is properly set up with appropriate imports, configuration, and test structure. I see that the mock server is now properly included, addressing the previous review comment about the missing file.
packages/platform/platform-multer/src/adapters/express/__mock__/Server.ts (1)
1-17: Mock server configuration looks good.The Server mock provides all necessary configuration for Express integration tests, including appropriate middleware, port configuration, and logger settings. The rootDir constant is properly set up to be replaced during build.
packages/platform/platform-multer/src/adapters/fastify/__mock__/Server.ts (1)
1-28: Fastify mock server configuration is well-structured.The Server mock provides all necessary configuration for Fastify integration tests, including appropriate plugins (@fastify/accepts, @fastify/cookie, fastify-raw-body, @fastify/formbody), port configuration, and logger settings. The fastify-raw-body plugin is correctly configured with options needed for proper form handling.
packages/platform/platform-multer/src/adapters/fastify/MulterModule.integration.spec.ts (1)
6-13: Consider removing type assertion for better type safety.The test setup looks good for integrating with the test SDK, but there's a potential type safety issue.
- adapter: PlatformFastify as any, + adapter: PlatformFastify,If the type assertion is necessary due to compatibility issues, consider adding a comment explaining why, or work toward making the types align properly.
packages/platform/platform-fastify/package.json (3)
4-4: Bump version alignment
The version bump to8.7.1is consistent with your broader monorepo updates. No issues found.
22-22: Confirm snapshot workflow removal
By removing the-uflag, you lose automatic snapshot updates in tests. Double-check if you intend to handle snapshot updates manually or via a separate command.
78-79: Validate dev dependencies
Confirm that using[email protected]and introducing[email protected]aligns with other tests and dependencies. Check for peer or version incompatibilities.packages/platform/platform-multer/src/adapters/koa/__mock__/Server.ts (2)
1-5: Imports verified
Imports for@tsed/ajvand@tsed/swaggeralign with typical Koa usage. No issues found.
9-10: Consistent directory setup
UsingrootDiris consistent with other mock servers, ensuring uniform file structure.packages/platform/platform-multer/package.json (1)
28-34: Add Fastify adapter export
The newly added./fastifyexport block is well-structured and matches existing patterns for Express and Koa. Great addition.packages/platform/platform-multer/src/adapters/koa/MulterModule.integration.spec.ts (2)
1-14: Looks clean and straightforward.No correctness or logical issues identified. Good job setting up the test utilities for the Koa adapter.
16-18: Consider adding more specific test cases.This repeats the previous suggestion to expand the test suite to cover different Multer functionalities (e.g., invalid file uploads, multiple file fields, etc.).
packages/platform/platform-multer/src/adapters/fastify/MulterModule.ts (2)
6-6: Use the “node:” protocol for Node.js built-in imports.Align with recommended best practices and the static analysis suggestion:
- import type {IncomingMessage} from "http"; + import type {IncomingMessage} from "node:http";🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
1-5: Overall implementation looks solid.The Fastify plugin and dynamic
multerimport mechanism are well-structured. The code effectively sets up multipart handling, ensures that request body/files are properly assigned, and integrates neatly with Fastify's plugin system.Also applies to: 7-78
tsconfig.spec.json (1)
12-78: New path mappings appear consistent and align with the Fastify adapter integration.No issues found. The entries match the corresponding source files and facilitate proper resolution for the new and existing modules.
| logger: { | ||
| level: "error" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Logger level is inconsistent with Express integration test.
The Express integration test uses a logger level of "off" while this test uses "error". Consider using consistent logger levels across integration tests for predictable behavior.
- level: "error"
+ level: "off"📝 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.
| logger: { | |
| level: "error" | |
| } | |
| logger: { | |
| - level: "error" | |
| + level: "off" | |
| } |
| @Configuration({ | ||
| port: 8083, | ||
| statics: { | ||
| "/": `${rootDir}/public` | ||
| }, | ||
| views: { | ||
| root: `${rootDir}/views`, | ||
| extensions: { | ||
| ejs: "ejs" | ||
| } | ||
| }, | ||
| middlewares: ["koa-compress", "koa-override", "koa-bodyparser"] | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider environment-based port configuration
Hardcoding port 8083 works for demos, but using an environment variable for the port is more flexible in production setups.
| $beforeRoutesInit() { | ||
| this.app.getApp().keys = ["some secret hurr"]; | ||
| this.app.use( | ||
| session( | ||
| { | ||
| key: "connect.sid" /** (string) cookie key (default is koa.sess) */, | ||
| /** (number || 'session') maxAge in ms (default is 1 day) */ | ||
| /** 'session' will result in a cookie that expires when session/browser is closed */ | ||
| /** Warning: If a session cookie is stolen, this cookie will never expire */ | ||
| maxAge: 86400000, | ||
| /** (boolean) automatically commit headers (default true) */ | ||
| overwrite: true, | ||
| /** (boolean) can overwrite or not (default true) */ | ||
| httpOnly: false, | ||
| /** (boolean) httpOnly or not (default true) */ | ||
| signed: false, | ||
| /** (boolean) signed or not (default true) */ | ||
| rolling: false, | ||
| /** (boolean) Force a session identifier cookie to be set on every response. The expiration is reset to the original maxAge, resetting the expiration countdown. (default is false) */ | ||
| renew: false, | ||
| /** (boolean) renew session when session is nearly expired, so we can always keep user logged in. (default is false)*/ | ||
| secure: false, | ||
| /** (boolean) secure cookie*/ | ||
| sameSite: undefined /** (string) session cookie sameSite options (default null, don't set it) */ | ||
| }, | ||
| this.app.rawApp as any | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secure session cookies
httpOnly is disabled, potentially leaving cookies exposed to XSS risks. Consider enabling HTTP-only cookies for safer session handling.
- httpOnly: false,
+ httpOnly: true,📝 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.
| $beforeRoutesInit() { | |
| this.app.getApp().keys = ["some secret hurr"]; | |
| this.app.use( | |
| session( | |
| { | |
| key: "connect.sid" /** (string) cookie key (default is koa.sess) */, | |
| /** (number || 'session') maxAge in ms (default is 1 day) */ | |
| /** 'session' will result in a cookie that expires when session/browser is closed */ | |
| /** Warning: If a session cookie is stolen, this cookie will never expire */ | |
| maxAge: 86400000, | |
| /** (boolean) automatically commit headers (default true) */ | |
| overwrite: true, | |
| /** (boolean) can overwrite or not (default true) */ | |
| httpOnly: false, | |
| /** (boolean) httpOnly or not (default true) */ | |
| signed: false, | |
| /** (boolean) signed or not (default true) */ | |
| rolling: false, | |
| /** (boolean) Force a session identifier cookie to be set on every response. The expiration is reset to the original maxAge, resetting the expiration countdown. (default is false) */ | |
| renew: false, | |
| /** (boolean) renew session when session is nearly expired, so we can always keep user logged in. (default is false)*/ | |
| secure: false, | |
| /** (boolean) secure cookie*/ | |
| sameSite: undefined /** (string) session cookie sameSite options (default null, don't set it) */ | |
| }, | |
| this.app.rawApp as any | |
| ) | |
| ); | |
| $beforeRoutesInit() { | |
| this.app.getApp().keys = ["some secret hurr"]; | |
| this.app.use( | |
| session( | |
| { | |
| key: "connect.sid" /** (string) cookie key (default is koa.sess) */, | |
| /** (number || 'session') maxAge in ms (default is 1 day) */ | |
| /** 'session' will result in a cookie that expires when session/browser is closed */ | |
| /** Warning: If a session cookie is stolen, this cookie will never expire */ | |
| maxAge: 86400000, | |
| /** (boolean) automatically commit headers (default true) */ | |
| overwrite: true, | |
| /** (boolean) can overwrite or not (default true) */ | |
| httpOnly: true, | |
| /** (boolean) httpOnly or not (default true) */ | |
| signed: false, | |
| /** (boolean) signed or not (default true) */ | |
| rolling: false, | |
| /** (boolean) Force a session identifier cookie to be set on every response. The expiration is reset to the original maxAge, resetting the expiration countdown. (default is false) */ | |
| renew: false, | |
| /** (boolean) renew session when session is nearly expired, so we can always keep user logged in. (default is false)*/ | |
| secure: false, | |
| /** (boolean) secure cookie*/ | |
| sameSite: undefined /** (string) session cookie sameSite options (default null, don't set it) */ | |
| }, | |
| this.app.rawApp as any | |
| ) | |
| ); |
Information
Todos
Summary by CodeRabbit
New Features
Documentation
Tests
Chores