feat: add rollup configuration and update dependencies for type defin… #249
Conversation
WalkthroughSwitches the post-package step to run Rollup to produce a bundled TypeScript declarations file, adds a Rollup DTS config and the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant FederationRequestService
participant makeSignedRequest
participant Response
Caller->>FederationRequestService: requestBinaryData(method, targetServer, endpoint, queryParams)
FederationRequestService->>makeSignedRequest: makeSignedRequest(...)
makeSignedRequest-->>FederationRequestService: Response
FederationRequestService->>Response: multipart()
Response-->>FederationRequestService: MultipartResult
FederationRequestService-->>Caller: Promise<MultipartResult>
note right of FederationRequestService: Signature updated to return MultipartResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewersPoem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
=======================================
Coverage 62.12% 62.12%
=======================================
Files 67 67
Lines 6390 6390
=======================================
Hits 3970 3970
Misses 2420 2420 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
rollup.config.js (10)
3-3: Translate comments to English.Comments should be in English for better maintainability and collaboration across the team.
Apply this diff:
-// Defina o ponto de entrada (entry point) do seu pacote +// Define the entry point for the package
6-6: Translate comments to English.Comments should be in English for better maintainability.
Apply this diff:
-// Função para determinar se um módulo deve ser tratado como externo +// Function to determine if a module should be treated as external
8-23: Consider expanding the Node.js core module list.The current list includes only a subset of Node.js core modules. Consider adding commonly used modules like
fs,path,url,crypto,buffer,util,os, andchild_processto ensure comprehensive external resolution.Apply this diff to expand the list:
if ( id.startsWith('node:') || [ 'http', 'https', 'dns', 'events', 'net', 'stream', 'tls', 'worker_threads', + 'fs', + 'path', + 'url', + 'crypto', + 'buffer', + 'util', + 'os', + 'child_process', ].includes(id) ) {
40-43: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Pacotes internos do monorepo - devem ser incluídos no bundle + // Internal monorepo packages - should be included in the bundle
45-48: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Imports relativos - devem ser incluídos no bundle + // Relative imports - should be included in the bundle
54-55: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Por padrão, trata como externo + // By default, treat as external
59-61: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Configuração para o arquivo JS (você pode continuar usando esbuild, se preferir) - // ... + // Configuration for JS file (you can continue using esbuild if preferred) + // ...
62-62: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Configuração para o arquivo DTS (definição de tipos) + // Configuration for DTS file (type definitions)
67-67: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- format: 'es', // Formato de saída para os tipos + format: 'es', // Output format for types
79-80: Translate comments to English.Comments should be in English for consistency.
Apply this diff:
- // Use a função isExternal para determinar quais módulos são externos + // Use the isExternal function to determine which modules are external
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
bundle.ts(1 hunks)package.json(1 hunks)packages/core/src/index.ts(1 hunks)packages/federation-sdk/src/services/federation-request.service.ts(2 hunks)rollup.config.js(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/federation-request.service.ts (2)
packages/core/src/index.ts (1)
MultipartResult(86-86)packages/core/src/utils/fetch.ts (1)
MultipartResult(6-10)
🔇 Additional comments (6)
packages/core/src/index.ts (1)
86-86: LGTM!The addition of
MultipartResultto the public API exports aligns with its usage in the federation SDK and properly exposes the type for consumers.packages/federation-sdk/src/services/federation-request.service.ts (2)
1-5: LGTM!The import of
MultipartResulttype aligns with the expanded public API from@rocket.chat/federation-core.
189-205: FetchResponse.multipart() is implemented correctly
Verified thatFetchResponse<T>inpackages/core/src/utils/fetch.tsdefinesmultipart(): Promise<MultipartResult>.turbo.json (1)
13-17: Verified referenced tsconfig files exist
tsconfig.sdk.types.json and tsconfig.base.json were found at the repository root.bundle.ts (1)
99-99: rollup.config.js verified
rollup.config.js is present at the repository root.package.json (1)
23-23: rollup-plugin-dts version validated
The range^6.2.3resolves to an existing npm version and no security advisories were found.
| // Dependências de terceiros | ||
| if ( | ||
| [ | ||
| 'zod', | ||
| 'mongodb', | ||
| 'tsyringe', | ||
| 'pino', | ||
| 'pino-std-serializers', | ||
| 'sonic-boom', | ||
| 'tweetnacl', | ||
| ].includes(id) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add reflect-metadata to the external dependencies list.
The package.json includes reflect-metadata as a dependency, but it's missing from the external list. This could lead to bundling issues.
Apply this diff:
if (
[
'zod',
'mongodb',
'tsyringe',
'pino',
'pino-std-serializers',
'sonic-boom',
'tweetnacl',
+ 'reflect-metadata',
].includes(id)
) {📝 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.
| // Dependências de terceiros | |
| if ( | |
| [ | |
| 'zod', | |
| 'mongodb', | |
| 'tsyringe', | |
| 'pino', | |
| 'pino-std-serializers', | |
| 'sonic-boom', | |
| 'tweetnacl', | |
| ].includes(id) | |
| ) { | |
| return true; | |
| } | |
| // Dependências de terceiros | |
| if ( | |
| [ | |
| 'zod', | |
| 'mongodb', | |
| 'tsyringe', | |
| 'pino', | |
| 'pino-std-serializers', | |
| 'sonic-boom', | |
| 'tweetnacl', | |
| 'reflect-metadata', | |
| ].includes(id) | |
| ) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In rollup.config.js around lines 25 to 38, the external dependencies array is
missing "reflect-metadata" which is declared in package.json and should not be
bundled; add 'reflect-metadata' to the array of module ids checked by includes
(alongside 'zod','mongodb', etc.) so Rollup treats it as external and avoids
bundling it into the output.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
20-24: Please moverollup-plugin-dtstodevDependencies.The DTS Rollup plugin is only needed during the build step, not at runtime. Keeping it in
dependenciesbloats production installs and cascades the package to downstream consumers. Relocate it underdevDependenciesalongside the rest of the build tooling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
bundle.ts(1 hunks)package.json(1 hunks)packages/core/src/index.ts(1 hunks)packages/federation-sdk/src/services/federation-request.service.ts(2 hunks)rollup.config.js(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- turbo.json
- bundle.ts
- packages/federation-sdk/src/services/federation-request.service.ts
- rollup.config.js
🔇 Additional comments (1)
packages/core/src/index.ts (1)
86-86: Approve public type export change
MultipartResultis defined inutils/fetch.tsand exposing it is an additive, non-breaking update required by federation-sdk.
…itions
Summary by CodeRabbit
New Features
Chores