-
-
Notifications
You must be signed in to change notification settings - Fork 12
perf: improve tsc performance for runtime project #97
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
Conversation
WalkthroughThe changes introduce explicit generic type parameters to operation handler instantiations in the client implementation, refactor a utility type using a mapped object for cleaner type mapping, and add explicit Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/runtime/src/client/client-impl.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/runtime/src/utils/type-utils.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/runtime/test/client-api/computed-fields.test.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/runtime/src/utils/type-utils.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
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.
Pull Request Overview
This PR aims to boost TypeScript compilation performance in the runtime project by making type parameters explicit and streamlining type mappings.
- Annotate operation handlers with
<Schema>to reduce compiler inference overhead. - Refactor
createPromisecalls to multi-line formatting for readability. - Replace a long conditional union in
MapBaseTypewith a conciseTypeMaplookup.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/runtime/test/client-api/computed-fields.test.ts | Added any annotations on eb callbacks in computed fields tests |
| packages/runtime/src/utils/type-utils.ts | Swapped union-based MapBaseType for a TypeMap mapping |
| packages/runtime/src/client/client-impl.ts | Added <Schema> generics to each operation handler and expanded createPromise calls |
Comments suppressed due to low confidence (3)
packages/runtime/test/client-api/computed-fields.test.ts:17
- Using
anyfor theebparameter weakens type safety in your tests. Consider importing and using the correct ExpressionBuilder type (or a typed stub) instead ofany.
upperName: (eb: any) => eb.fn('upper', ['name']),
packages/runtime/test/client-api/computed-fields.test.ts:93
- Similar to above, avoid
anyon theebparameter. Use the actual builder type or a minimal mock interface to keep the test type-checked.
upperName: (eb: any) => eb.lit(null),
packages/runtime/src/client/client-impl.ts:417
- [nitpick] This pattern of calling
createPromisewith a new handler is repeated across many methods. Consider extracting a helper function that takes the operation name and handler class to reduce boilerplate and ease future updates.
return createPromise(
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit