feat: support cjs and esm both by tshy #67
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request updates various configuration files and refactors the multipart file handling. Changes include modifications to ESLint and GitHub Actions settings, removal of legacy multipart implementations and associated TypeScript definitions, and updates to ignore files. A new implementation for multipart processing is introduced along with enhanced error handling, updated context methods, and refined utility functions. The package metadata has been updated and new testing and TypeScript configuration files have been added. These changes collectively modernize the codebase and improve type safety. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Context
participant AppCore
Client->>Middleware: Send multipart request
Middleware->>Middleware: Check if request is multipart
alt Multipart request valid
Middleware->>Context: Call saveRequestFiles()
Context-->>Middleware: Process file saving
else Non-multipart or non-matching request
Middleware->>Middleware: Skip file processing
end
Middleware->>AppCore: Pass control to next middleware
sequenceDiagram
participant EggCore
participant AppBootHook
participant Logger
EggCore->>AppBootHook: Instantiate and call configWillLoad()
AppBootHook->>AppBootHook: Normalize multipart options
AppBootHook->>Logger: Log multipart mode & temp directory info
AppBootHook->>EggCore: Add 'multipart' to middleware configuration
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/app/extend/context.ts (3)
8-9: Consider adding type declarations forco-busboy.
You may create a custom.d.tsfile or install an existing type declaration library if available. This ensures better type safety.
169-169: Avoid unnecessarythisalias.
You can directly referencethiswithin methods, even in arrow functions, reducing confusion and aligning with recommended TypeScript patterns.Example fix:
-// eslint-disable-next-line @typescript-eslint/no-this-alias -const ctx = this; +// Use `this` directly instead🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
343-343: Split the assignment to avoid confusion.
Assignments within a variable declaration expression can reduce readability. Consider splitting them into separate statements.Here’s a potential fix:
- const limits: Record<string, number | undefined> = opts.limits = { ...options.limits }; + opts.limits = { ...options.limits }; + const limits: Record<string, number | undefined> = opts.limits;🧰 Tools
🪛 Biome (1.9.4)
[error] 343-343: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/app/middleware/multipart.ts (1)
8-11: Remove or implement the commented configuration.The commented line suggests incomplete configuration for
pathToRegexpModule.Either remove the comment or implement the configuration:
const matchFn = options.fileModeMatch && pathMatching({ match: options.fileModeMatch, - // pathToRegexpModule: app.options.pathToRegexpModule, });src/app.ts (1)
7-18: Enhance type safety and error handling in configWillLoad.The method could benefit from explicit return type and error handling for configuration normalization.
Apply this diff to improve the implementation:
- configWillLoad() { + configWillLoad(): void { + try { this.app.config.multipart = normalizeOptions(this.app.config.multipart); const options = this.app.config.multipart; this.app.coreLogger.info('[@eggjs/multipart] %s mode enable', options.mode); if (options.mode === 'file' || options.fileModeMatch) { this.app.coreLogger.info('[@eggjs/multipart] will save temporary files to %j, cleanup job cron: %j', options.tmpdir, options.cleanSchedule.cron); // enable multipart middleware this.app.config.coreMiddleware.push('multipart'); } + } catch (err) { + this.app.coreLogger.error('[@eggjs/multipart] failed to load config: %s', err); + throw err; + } }test/wrong-mode.test.ts (1)
4-27: Add more test cases for configuration validation.Consider adding test cases for other configuration scenarios:
- Invalid tmpdir path
- Invalid cleanSchedule configuration
- Missing required options
Example test case:
it('should start fail when tmpdir is not writable', async () => { app = mm.app({ baseDir: 'apps/wrong-tmpdir', }); await assert.rejects(async () => { await app.ready(); }, /tmpdir is not writable/); });src/app/schedule/clean_tmpdir.ts (3)
1-6: Improve type safety by specifying a more precise return type.The return type
anycould be replaced with a more specific type.Apply this diff to improve type safety:
-export default (app: EggCore): any => { +export default (app: EggCore): typeof CleanTmpdir => {
17-30: Enhance error logging with more details.Consider adding more context to error logs to aid in debugging.
Apply this diff to improve error logging:
} catch (err) { /* c8 ignore next 3 */ - ctx.coreLogger.error('[@eggjs/multipart:CleanTmpdir:error] remove tmpdir: %j error: %s', dir, err); + ctx.coreLogger.error('[@eggjs/multipart:CleanTmpdir:error] remove tmpdir: %j error: %s\nStack: %s', + dir, err.message, err.stack); ctx.coreLogger.error(err); }
32-53: Consider performance and configurability improvements.
- The sequential directory removal could be parallelized for better performance.
- The cleanup periods (1 year, 3 months, 7 days) could be made configurable.
Example implementation:
interface CleanupConfig { years: number; months: number; days: number; } async subscribe() { const { ctx } = this; const config = ctx.app.config; const cleanupConfig = config.multipart.cleanupConfig || { years: 1, months: 3, days: 7, }; ctx.coreLogger.info('[@eggjs/multipart:CleanTmpdir] start clean tmpdir: %j', config.multipart.tmpdir); const tasks = []; // years for (let i = 1; i <= cleanupConfig.years; i++) { const date = dayjs().subtract(i, 'years'); const dir = path.join(config.multipart.tmpdir, date.format('YYYY')); tasks.push(this._remove(dir)); } // months and days (similar implementation) await Promise.all(tasks); ctx.coreLogger.info('[@eggjs/multipart:CleanTmpdir] end'); }src/lib/utils.ts (1)
72-85: Consider improving error handling and type safety.
- The
void | Errorunion type could be improved.- The
anytype forfileStreamshould be more specific.- Error handling could be more robust.
Consider these improvements:
-options.checkFile = (_fieldName: string, fileStream: any, fileName: string): void | Error => { +interface FileStream { + readable: boolean; + // Add other expected properties +} + +options.checkFile = (_fieldName: string, fileStream: FileStream | null, fileName: string): Error | undefined => { // just ignore, if no file - if (!fileStream || !fileName) return; + if (!fileStream || !fileName) return undefined; try { if (!checkExt(fileName)) { - const err = new Error('Invalid filename: ' + fileName); - Reflect.set(err, 'status', 400); + const err = new Error('Invalid filename: ' + fileName) as Error & { status?: number }; + err.status = 400; return err; } + return undefined; } catch (err: any) { err.status = 400; return err; } };🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/config/config.default.ts (1)
92-98: Consider improving type safety in checkFile signature.The
checkFilemethod signature could be improved:
- The
anytype forfileparameter should be more specific- The
void | Errorunion type could be clearerConsider this improvement:
- checkFile?( - fieldname: string, - file: any, - filename: string, - encoding: string, - mimetype: string - ): void | Error; + checkFile?( + fieldname: string, + file: { + readable: boolean; + // Add other expected properties + } | null, + filename: string, + encoding: string, + mimetype: string + ): Error | undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.eslintignore(1 hunks).eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)app.js(0 hunks)app/middleware/multipart.js(0 hunks)config/config.default.js(0 hunks)index.d.ts(0 hunks)package.json(2 hunks)src/app.ts(1 hunks)src/app/extend/context.ts(11 hunks)src/app/middleware/multipart.ts(1 hunks)src/app/schedule/clean_tmpdir.ts(4 hunks)src/config/config.default.ts(1 hunks)src/index.ts(1 hunks)src/lib/LimitError.ts(1 hunks)src/lib/MultipartFileTooLargeError.ts(1 hunks)src/lib/utils.ts(3 hunks)src/typings/index.d.ts(1 hunks)test/wrong-mode.test.js(0 hunks)test/wrong-mode.test.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (5)
- app.js
- app/middleware/multipart.js
- config/config.default.js
- test/wrong-mode.test.js
- index.d.ts
✅ Files skipped from review due to trivial changes (5)
- src/index.ts
- .eslintignore
- .gitignore
- src/typings/index.d.ts
- tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/config/config.default.ts
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/lib/utils.ts
[error] 72-72: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/app/extend/context.ts
[error] 73-73: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 169-169: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 343-343: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
🔇 Additional comments (21)
src/lib/LimitError.ts (1)
1-13: Looks good.
The error class is well-structured, with a clearcodeproperty and413status code, making it straightforward for consumers to handle limit-related errors.src/lib/MultipartFileTooLargeError.ts (1)
1-15: Implementation is appropriate for large file handling.
This custom error clearly communicates context about the file that triggered the size limit, aiding debugging and error handling.src/lib/utils.ts (2)
1-4: LGTM! Module imports updated to ESM syntax.The conversion from CommonJS to ESM imports aligns with modern JavaScript practices.
32-37: LGTM! Well-structured type-safe helper function.The
humanizeBytesfunction effectively encapsulates the byte conversion logic with proper type checking.src/config/config.default.ts (1)
101-123: LGTM! Well-structured default configuration.The default configuration values are sensible and well-documented. The use of
as MultipartConfigensures type safety..eslintrc (1)
2-5: LGTM! Enhanced ESLint configuration.The updated ESLint configuration properly supports TypeScript and enforces Node.js prefix conventions.
.github/workflows/nodejs.yml (1)
15-15: LGTM! Updated Node.js version requirements.Removing Node.js 16 and testing against versions 18, 20, and 22 aligns with the PR objective to drop support for versions earlier than 18.19.0.
package.json (14)
2-2: Package Name Update
The package name has been updated to a scoped name ("@eggjs/multipart"), which is beneficial for proper package organization and namespace management.
4-6: Publish Configuration Added
A new"publishConfig"field with"access": "public"has been introduced. This ensures that when the package is published, it will have the intended public access settings.
11-16: EggPlugin Exports Configuration
The"eggPlugin"section now includes an"exports"object that specifies module entry points for"import","require", and"typescript". This clearly delineates the responsibilities for both ESM and CommonJS consumers.
21-21: Repository URL Update
The repository URL has been updated to"git+https://github.com/eggjs/multipart.git", ensuring that the package metadata reflects the correct source repository.
34-34: Homepage Update
The"homepage"field now points to"https://github.com/eggjs/multipart#readme", aligning it with the updated repository information.
36-36: Node.js Engine Requirement Update
The"engines"field now requires Node.js version>= 18.19.0, effectively dropping support for older Node.js versions. This is a breaking change—please ensure that the documentation and downstream dependencies are updated accordingly.
39-43: Dependencies Update
The dependencies have been revised to include new packages (e.g.,"@eggjs/core") and update versions for others such as"bytes","co-busboy", and"egg-path-matching". Verify that these versions are compatible with the new Node.js engine requirement and the overall project architecture.
46-65: DevDependencies Update
A comprehensive update of the devDependencies has been performed, adding tools like"@arethetypeswrong/cli","@eggjs/bin","@eggjs/mock", and others. These changes aim to improve the development workflow and type safety. Please double-check that no conflicts arise with the existing build process.
66-74: Scripts Restructuring
The scripts have been restructured to include linting, pretesting, testing, CI, and cleanup commands. The"prepublishOnly"script now runs a sequence of commands:"tshy && tshy-after && attw --pack". Please verify that theattwcommand is valid and that any necessary dependency for it is included, as it does not appear in the current dependency lists.
76-76: Module Type Declaration
The addition of"type": "module"enforces an ES module format, which aligns with the updated dual-support strategy for ESM and CommonJS.
77-82: Tshy Configuration Addition
A new"tshy"configuration object has been added that maps the package’s primary export to"./src/index.ts". This setup is presumably used by the build or bundling process. Please ensure that this configuration is correctly integrated with your toolchain.
83-95: Exports Field for Dual Module Support
The"exports"field provides explicit entry points for both ECMAScript modules and CommonJS, along with corresponding TypeScript definitions. This robust configuration is key to supporting both module systems; however, please verify that the provided file paths (for both ESM and CJS builds) match the actual build output.
96-99: Files Field Update
The"files"array now includes only"dist"and"src", ensuring that only necessary files are published while excluding extraneous ones. This improves package security and distribution size.
100-101: Main and Types Fields Update
The updates to the"types"and"main"fields now correctly point to"./dist/commonjs/index.d.ts"and"./dist/commonjs/index.js", respectively. This change confirms that the CommonJS build is now the default for environments that do not support ESM natively.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/multipart.test.ts (1)
389-393: Remove duplicate before hook.There are duplicate
beforehooks in the "upload one file" test suite. Combine them into a single hook to improve maintainability.- before(async () => { - await app.ready(); - server = app.listen(); - host = 'http://127.0.0.1:' + server.address().port; - }); - before(async () => { - await app.httpRequest() - .get('/upload') - .expect(200); - }); + before(async () => { + await app.ready(); + server = app.listen(); + host = 'http://127.0.0.1:' + server.address().port; + await app.httpRequest() + .get('/upload') + .expect(200); + });
🧹 Nitpick comments (9)
src/lib/utils.ts (3)
32-36: Handle potential null returns frombyteslibrary
If thebytesfunction fails to parse the input, it can returnnull. Consider handling that scenario more gracefully, for instance by throwing an error or defaulting to zero.- return bytes(size) as number; + const result = bytes(size); + if (result === null) { + throw new Error(`Invalid size input: ${size}`); + } + return result;
76-76: UseError | undefinedinstead ofvoid | Errorfor clarity
Returningvoidin a union type may be confusing in TypeScript. UsingError | undefinedmakes the intent more explicit and aligns better with common conventions.- options.checkFile = (_fieldName: string, fileStream: any, fileName: string): void | Error => { + options.checkFile = (_fieldName: string, fileStream: any, fileName: string): Error | undefined => {🧰 Tools
🪛 Biome (1.9.4)
[error] 76-76: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
82-86: Use a consistent approach to set error status
Currently, line 82 sets the error status usingReflect.set(err, 'status', 400);while line 86 uses direct assignmenterr.status = 400;. Consider standardizing the approach for consistency.test/fixtures/apps/whitelist-function/app/controller/upload.js (2)
8-8: Refactor the while loop condition for better readability.The assignment within the while condition makes the code harder to read and maintain. Consider restructuring using async iteration.
- while ((part = await parts()) != null) { + for await (const part of parts()) { if (Array.isArray(part)) { continue; } else { break; } }🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
16-17: Consider using async streams with pipeline.For better error handling and memory management, consider using the stream pipeline API.
- const ws = fs.createWriteStream(path.join(this.app.config.logger.dir, 'multipart-test-file')); - part.pipe(ws); + const { pipeline } = require('node:stream/promises'); + await pipeline( + part, + fs.createWriteStream(path.join(this.app.config.logger.dir, 'multipart-test-file')) + );test/dynamic-option.test.ts (1)
37-37: Consider using a proper type for form stream.Instead of casting to
any, consider creating a proper type definition for the form stream.interface FormStream extends NodeJS.ReadableStream { headers(): { [key: string]: string }; }Then use it like:
- stream: form as any, + stream: form as FormStream,test/ts.test.ts (1)
16-25: Consolidate setup hooks for better maintainability.Multiple
beforehooks can be consolidated into a single hook.- before(() => { - app = mm.app({ - baseDir: 'apps/ts', - }); - return app.ready(); - }); - before(() => { - server = app.listen(); - host = 'http://127.0.0.1:' + server.address().port; - }); + before(async () => { + app = mm.app({ + baseDir: 'apps/ts', + }); + await app.ready(); + server = app.listen(); + host = 'http://127.0.0.1:' + server.address().port; + });🧰 Tools
🪛 Biome (1.9.4)
[error] 22-25: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/multipart-for-await.test.ts (1)
40-40: Consolidate form stream type casting.Multiple instances of
as anytype casting can be improved by defining a proper type.Create a types file (e.g.,
test/types.ts) with:import formstream from 'formstream'; declare module 'formstream' { interface FormStream extends NodeJS.ReadableStream { headers(): { [key: string]: string }; } interface FormStreamConstructor { new(): FormStream; (): FormStream; } const formstream: FormStreamConstructor; export = formstream; }Then remove all
as anycasts from the test files.Also applies to: 64-64, 82-82, 100-100, 119-119, 141-141
test/file-mode.test.ts (1)
67-67: Consider using a more specific type for the stream property.The stream property is consistently cast to
anytype across multiple test cases. Consider creating a proper type for the form stream to improve type safety.-stream: form as any, +stream: form as FormStream,Add the following type definition at the top of the file:
interface FormStream extends NodeJS.ReadableStream { headers(): { [key: string]: string }; }Also applies to: 103-103, 125-125, 161-161, 188-188, 206-206, 221-221, 238-238, 257-257, 276-276, 294-294, 312-312, 331-331, 410-410, 428-428, 514-514, 535-535, 551-551, 570-570, 588-588, 633-633, 655-655, 677-677
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
package.json(2 hunks)src/app/middleware/multipart.ts(1 hunks)src/lib/utils.ts(1 hunks)test/dynamic-option.test.ts(3 hunks)test/file-mode.test.ts(22 hunks)test/fixtures/apps/ts/config/config.default.ts(2 hunks)test/fixtures/apps/whitelist-function/app/controller/upload.js(1 hunks)test/fixtures/apps/whitelist-function/app/router.js(0 hunks)test/multipart-for-await.test.ts(8 hunks)test/multipart.test.ts(40 hunks)test/ts.test.js(0 hunks)test/ts.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- test/fixtures/apps/whitelist-function/app/router.js
- test/ts.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/middleware/multipart.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/apps/whitelist-function/app/controller/upload.js
[error] 8-8: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
test/ts.test.ts
[error] 22-25: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 29-29: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 30-30: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/lib/utils.ts
[error] 76-76: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
test/multipart.test.ts
[error] 393-397: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
🔇 Additional comments (11)
test/fixtures/apps/ts/config/config.default.ts (2)
1-15: Well-implemented type definition with clear documentation!The
PowerPartial<T>type implementation is clean and follows TypeScript best practices. The recursive type correctly handles nested objects, making all properties optional at any depth. The documentation with examples is particularly helpful.
24-26: Consider expanding multipart configuration options.Given that this PR focuses on enhancing multipart functionality and adding CJS/ESM support, the multipart configuration might benefit from additional options to control module-specific behavior and file handling settings.
Let's check what options are available in the new implementation:
✅ Verification successful
Expand Multipart Configuration Options
- The test fixture (
test/fixtures/apps/ts/config/config.default.ts) only setsconfig.multipartto{ mode: 'file' }, while example usages in the repository (e.g. in the README and in conditions withinsrc/app.ts) indicate additional options liketmpdir,cleanSchedule, andfileModeMatchexist.- The interface defined in
src/config/config.default.tsforMultipartConfigdocuments further properties that can be leveraged for enhanced multipart handling, which could be beneficial given the PR’s focus on extended multipart functionality and CJS/ESM support.Consider reviewing and expanding the fixture’s multipart configuration to incorporate these options to ensure consistency and full functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for multipart configuration interface or type definitions ast-grep --pattern 'interface $name { $$$ }' | grep -A 10 'Multipart\|Config' # Search for usage examples of multipart configuration rg -A 5 'multipart.*=.*{' test/Length of output: 209
Script:
#!/bin/bash echo "Searching for config.multipart assignments in configuration files:" rg -A 5 "config\.multipart" test/fixtures/apps/ts/config/config.default.ts echo "Searching for usage examples of multipart configuration in the repository:" rg -A 5 'config\.multipart\s*=' . echo "Searching for definitions of interfaces containing 'Multipart':" rg -A 5 'interface\s+.*Multipart' .Length of output: 3262
package.json (3)
36-36: Acknowledge the breaking change to Node.js versions
Raising the required version from Node.js 14 to 18.19.0 is a significant change. Ensure the project documentation and announcements convey this break in backward compatibility.
66-75: Comprehensive scripts configuration
These scripts cover linting, running tests, cleaning, and publishing tasks. The workflow is well-structured for continuous integration and development.
83-95: Good use of conditional exports
Providing separate ESM and CommonJS exports, along with a dedicated TypeScript entry, ensures broad compatibility and a smooth developer experience.test/multipart-for-await.test.ts (1)
128-131: Investigate and document the skipped fieldNameSize test.The TODO comment references busboy 1.x limitations. Let's verify if this limitation still exists in the current version.
✅ Verification successful
Update the skipped test for fieldNameSize
Investigation shows that the installed busboy (v1.6.0) documents support forfieldNameSize(with a default of 100 bytes). This indicates that the original limitation noted in the TODO is likely outdated. Please verify that egg-multipart now handlesfieldNameSizecorrectly and, if so, remove the skip on this test and update the test case accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current busboy version and relevant changes npm list busboy # Check if the limitation is documented in current version curl -s https://raw.githubusercontent.com/mscdex/busboy/master/README.md | grep -A 5 fieldNameSizeLength of output: 2143
test/file-mode.test.ts (3)
1-10: LGTM! Clean import statements.The imports are well-organized, using the node: protocol for Node.js built-in modules and direct imports for third-party packages.
15-17: LGTM! Proper type annotations.Type annotations have been correctly added for variables:
app: MockApplicationserver: anyhost: string
349-350: LGTM! Modern async/await usage.The code uses
scheduler.waitfromnode:timers/promisesinstead of a custom sleep function, which is a better approach for handling delays in tests.test/multipart.test.ts (2)
15-17: LGTM! Consistent type annotations.Type annotations have been correctly and consistently added for variables across all test suites:
app: MockApplicationserver: anyhost: stringAlso applies to: 251-253, 315-317, 379-381, 599-601
118-119: LGTM! Modern async/await usage.The code uses
scheduler.waitfromnode:timers/promisesinstead of a custom sleep function, which is a better approach for handling delays in tests.Also applies to: 558-559
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/file-mode-limit-filesize-per-request.test.ts (2)
9-9: Consider using a more specific type for server variable.Instead of using
any, consider usingServerfrom the 'node:http' module for better type safety.- let server: any; + let server: import('node:http').Server;
37-37: Consider extracting repeated type assertions into a helper function.The
as anytype assertion for the form stream is repeated across multiple test cases. Consider extracting this into a helper function for better maintainability.// Add at the top of the file function createRequestOptions(form: formstream, options: Partial<urllib.RequestOptions> = {}) { return { method: 'POST', headers: form.headers(), stream: form as any, ...options, }; } // Usage in tests const res = await urllib.request(host + '/upload-limit-1mb', createRequestOptions(form));Also applies to: 61-61, 77-77, 102-102
test/stream-mode-with-filematch.test.ts (1)
49-49: Replaceanytype assertions with proper types.Using
as anytype assertions can mask potential type incompatibilities. Consider defining proper types for the form stream.// Add this type definition at the top of the file type FormStream = ReturnType<typeof formstream>; // Then replace the type assertions - stream: form as any, + stream: form as FormStream,Also applies to: 90-90, 113-113
test/enable-pathToRegexpModule.test.ts (1)
46-46: Consider using a proper type instead ofany.The test file has multiple instances of type casting
formtoany. This reduces type safety. Consider creating a proper type for the form stream to avoid usingany.// Add this type definition at the top of the file type FormStream = ReturnType<typeof formstream>; // Then replace the type casting with stream: form as FormStreamAlso applies to: 87-87, 128-128
test/stream-mode-with-filematch-glob.test.ts (4)
14-14: Consider using a more specific type for server.Instead of using
any, consider usinghttp.Servertype from Node.js for better type safety.- let server: any; + let server: import('node:http').Server;
49-49: Consider using a more specific type for form stream.Instead of casting to
any, consider creating a proper type for the form stream.- stream: form as any, + stream: form as formstream.Form,
75-114: Consider extracting common test setup code.Both test cases share identical setup code for form creation and validation. Consider extracting this into a helper function to improve maintainability and reduce duplication.
async function createAndSubmitForm(url: string) { const form = formstream(); form.field('foo', 'fengmk2').field('love', 'egg'); form.file('file1', __filename, 'foooooooo.js'); form.file('file2', __filename); form.buffer('file3', Buffer.from(''), '', 'application/octet-stream'); form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js')); form.field('work', 'with Node.js'); const headers = form.headers(); return urllib.request(host + url, { method: 'POST', headers, stream: form as formstream.Form, }); } async function validateResponse(res: urllib.HttpResponse, app: MockApplication) { assert.equal(res.status, 200); const data = JSON.parse(res.data); // ... rest of the validation logic }
131-131: Apply consistent type handling across all test cases.For consistency with previous suggestions, update the type casting here as well.
- stream: form as any, + stream: form as formstream.Form,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/enable-pathToRegexpModule.test.ts(5 hunks)test/file-mode-limit-filesize-per-request.test.ts(6 hunks)test/stream-mode-with-filematch-glob.test.ts(5 hunks)test/stream-mode-with-filematch.test.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
🔇 Additional comments (9)
test/file-mode-limit-filesize-per-request.test.ts (2)
1-5: LGTM! Well-organized imports.The transition to ES modules and TypeScript is clean and well-structured, with proper organization of imports (built-ins first, followed by external packages).
29-121: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Success cases for files within size limits
- Error cases for files exceeding limits
- Input validation for content type
- Proper cleanup of temporary files
The assertions are well-structured and verify all important aspects of the responses.
test/stream-mode-with-filematch.test.ts (2)
1-10: LGTM! Clean migration to ESM imports.The migration from CommonJS to ESM is well-executed with proper organization of imports and correct ESM-compatible filename/dirname setup.
12-32: Verify @eggjs/mock compatibility with the test suite.The migration from egg-mock to @eggjs/mock looks good, but let's verify the compatibility and feature parity between the libraries.
✅ Verification successful
@eggjs/mock integration has been successfully verified.
- The npm view shows that @eggjs/mock is up-to-date and reliable.
- Searches confirmed that the codebase exclusively uses @eggjs/mock across the tests.
- There are no lingering references to egg-mock, and the API usage in the test files (including
test/stream-mode-with-filematch.test.ts) aligns well with @eggjs/mock's expected behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if @eggjs/mock provides all the required features used in the test # Check if the package exists and get its details echo "Checking @eggjs/mock package..." npm view @eggjs/mock # Search for any usage of egg-mock features in the codebase that might not be available in @eggjs/mock echo "Checking for potentially unsupported features..." rg -l 'egg-mock' . rg -l '@eggjs/mock' .Length of output: 1814
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-25: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 29-29: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 30-30: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/enable-pathToRegexpModule.test.ts (3)
9-11: LGTM! Type annotations improve type safety.The addition of TypeScript types for
app,server, andhostvariables improves type safety and code maintainability.
13-16: Verify if the commented configuration can be removed.The
pathToRegexpModuleconfiguration is commented out. If this configuration is no longer needed, consider removing it entirely rather than leaving it commented.✅ Verification successful
Remove the commented-out configuration for clarity.
The repository search confirms that the
pathToRegexpModuleconfiguration only appears as commented-out code (both in the test and insrc/app/middleware/multipart.ts) with no active usage. Its reference in the CHANGELOG suggests past support but no current reliance in the code. Therefore, it is safe to remove this commented configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of pathToRegexpModule rg "pathToRegexpModule" -g "!test/enable-pathToRegexpModule.test.ts"Length of output: 496
8-8: Verify if the test suite should remain skipped.The test suite is currently disabled with
.skip. Please verify if this is intentional and if there's a tracking issue for re-enabling these tests.✅ Verification successful
Test suite intentionally skipped pending configuration update
The evidence confirms that the test suite in
test/enable-pathToRegexpModule.test.tsremains disabled via.skip. The commented configuration line (// pathToRegexpModule: require.resolve('path-to-regexp-v8')) suggests that re-enabling is anticipated only after additional work or updates. No further tracking issue or context was found in the repository to indicate an immediate plan to re-enable the tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related issues or TODOs about skipped tests rg -i "enable-pathToRegexpModule.*skip" -g "!test/enable-pathToRegexpModule.test.ts"Length of output: 86
Script:
#!/bin/bash # Display the first 30 lines of the test file to inspect any comments or reasoning for skipping the suite sed -n '1,30p' test/enable-pathToRegexpModule.test.ts # Additionally, search the test file for keywords that might explain why the test suite is skipped grep -Ei "(skip|TODO|disable|temporary)" test/enable-pathToRegexpModule.test.ts || echo "No skip-related comments found"Length of output: 1084
test/stream-mode-with-filematch-glob.test.ts (2)
1-10: LGTM! Well-structured imports following Node.js best practices.The import statements follow best practices by using the 'node:' prefix for built-in modules and properly handling ESM __filename/__dirname.
65-65: Verify the MIME type for TypeScript files.The MIME type
video/mp2tfor a TypeScript file seems incorrect. TypeScript files typically useapplication/typescriptortext/typescript.
[skip ci] ## [4.0.0](v3.5.0...v4.0.0) (2025-02-03) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new middleware for handling multipart requests. - Added improved error handling for oversized file uploads. - **Refactor** - Streamlined configuration and context enhancements for better stability and TypeScript support. - Modernized the codebase by transitioning to ES modules and updating type definitions. - **Chores** - Updated package metadata, dependencies, and continuous integration settings to support newer Node.js versions. - Introduced a new TypeScript configuration for stricter type-checking. - **Tests** - Added unit tests to validate application behavior under incorrect configurations. - Established comprehensive tests for multipart form handling to ensure correct processing of file uploads. - Transitioned existing tests to TypeScript and updated assertions for consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#67](#67)) ([ccefb3e](ccefb3e))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
New Features
Refactor
Chores
Tests