-
Notifications
You must be signed in to change notification settings - Fork 1.4k
refactor: Install rest-typings and migrate helper types #6876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
refactor: Install rest-typings and migrate helper types #6876
Conversation
WalkthroughThis PR removes local REST API type definitions for PaginatedRequest and PaginatedResult, replacing them with re-exports from an external typings package. Two new dependencies are added to package.json to support this migration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)
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 |
39693ae to
1c1e6dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/verify-migration-phase.sh (1)
1-52: Consider adding test execution to match documentation.MIGRATION_SUMMARY.md and MIGRATION_CHECKLIST.md both reference running
yarn testas part of the verification process, but this script only checks TypeScript compilation and linting. Consider adding test execution or updating the documentation to reflect the actual verification steps.🔎 Proposed addition of test execution
echo "✅ Linting passed with 0 errors" +# Run tests +echo "🧪 Running tests..." +if ! yarn test; then + echo "❌ Tests failed!" + exit 1 +fi +echo "✅ Tests passed" + echo "" echo "✅ Phase $PHASE verification complete!" echo " - TypeScript: ✅" echo " - Linting: ✅ (0 errors)" +echo " - Tests: ✅"
📜 Review details
Configuration used: Organization 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.gitignoreMIGRATION_CHECKLIST.mdMIGRATION_PLAN.mdMIGRATION_SUMMARY.mdapp/definitions/rest/COMPATIBILITY_LAYER.mdapp/definitions/rest/helpers/PaginatedRequest.tsapp/definitions/rest/helpers/PaginatedResult.tsapp/definitions/rest/helpers/index.tspackage.jsonscripts/verify-migration-phase.sh
🔇 Additional comments (8)
app/definitions/rest/COMPATIBILITY_LAYER.md (1)
1-52: Documentation looks good, but note the .gitignore conflict.The compatibility layer documentation is well-structured and provides clear guidance with helpful examples. However, this file is listed in
.gitignore(line 92), which conflicts with it being committed. Please refer to the review comment on.gitignorefor resolution.MIGRATION_PLAN.md (1)
1-315: Comprehensive migration plan, but conflicts with .gitignore.The migration plan is well-structured with clear phases, time estimates, and quality gates. However, this file matches the
*MIGRATION*.mdpattern in.gitignore(line 88), which would prevent it from being tracked. Please refer to the review comment on.gitignorefor resolution.app/definitions/rest/helpers/PaginatedResult.ts (1)
1-2: No action needed—verification confirms the external type exists and all usages are type-safe.The
@rocket.chat/rest-typingspackage (v6.13.1) is properly declared in package.json, and the re-export is working correctly across all seven files that importPaginatedResult(directory.ts, settings.ts, teams.ts, videoConference.ts, omnichannel.ts, emojiCustom.ts, chat.ts). No type errors or import failures are present, confirming structure compatibility and type-safety.package.json (1)
50-53: The specified versions (6.13.1) exist, are published, and have no known security vulnerabilities. They are compatible by design, released together as matched versions in Rocket.Chat 6.13.1. The exact version pinning indicates intentional selection for stable compatibility with a specific Rocket.Chat server release, which is appropriate for a mobile client targeting that version. No action needed.app/definitions/rest/helpers/index.ts (1)
2-4: LGTM! Clear migration plan documented.The TODO comments effectively communicate the migration strategy and rationale for maintaining the current implementation during the transition phase.
MIGRATION_SUMMARY.md (1)
1-58: Well-structured migration documentation.The summary provides a clear overview of the migration process with helpful quick references, phase descriptions, quality gates, and important notes. The structure will guide developers effectively through the phased migration.
MIGRATION_CHECKLIST.md (1)
1-288: Excellent comprehensive migration checklist.This checklist is well-structured and provides clear, actionable steps for each migration phase. The inclusion of verification commands (
yarn lint,tsc --noEmit) after each category and standardized commit messages will help maintain quality and consistency throughout the migration process.app/definitions/rest/helpers/PaginatedRequest.ts (1)
1-2: No action needed. The re-export to@rocket.chat/rest-typingsis confirmed compatible.The migration refactored the local
PaginatedRequestdefinition to use the external package type, which correctly supports generic parametersT(request body) and an optionalS(sort field) as evidenced by usage patterns throughout the codebase (PaginatedRequest<{ query: string }, 'name'>,PaginatedRequest<{ text: string }>). The external type includes the expected fields:count,offset,sort, andquery. The codebase actively uses this re-export without type errors across multiple endpoint definitions.
f39f83e to
861a4fa
Compare
Preparation & Setup - Install @rocket.chat/[email protected] - Install @rocket.chat/[email protected] (required dependency) - Explore and document package structure Helper Types Migration - Migrate PaginatedRequest to re-export from @rocket.chat/rest-typings - Migrate PaginatedResult to re-export from @rocket.chat/rest-typings - Update helpers/index.ts with TODO for future endpoint migration - Keep custom ResultFor, Serialized, ErrorResult for SDK compatibility
861a4fa to
d9b821b
Compare
diegolmello
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Left a few comments.
| @@ -1,4 +1,7 @@ | |||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | |||
| // TODO: Once we migrate all endpoints to use @rocket.chat/rest-typings, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do it on this PR or start the work on this PR, so it serves as a POC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure...will try my best...
Preparation & Setup
Helper Types Migration
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.