-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Enable the reactCompiler #363
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 an experimental React compiler in the web service by adding Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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
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.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/webservice/package.json(3 hunks)apps/webservice/tsconfig.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/package.json
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: Test
- GitHub Check: build (linux/amd64)
- GitHub Check: Format
🔇 Additional comments (1)
pnpm-workspace.yaml (1)
13-13: Confirm Compatibility for TypeScript Upgrade
The TypeScript version update from “^5.6.3” to “^5.8.2” is aligned with the overall performance improvements expected with the experimental React compiler. Please ensure that all packages and build tools are compatible with the new TypeScript version to avoid unexpected type errors or compilation issues.
apps/webservice/tsconfig.json
Outdated
| "include": [".", ".next/types/**/*.ts"], | ||
| "exclude": ["node_modules"] | ||
| "exclude": ["node_modules", ".next/types"] |
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.
💡 Verification agent
❓ Verification inconclusive
Revisit the Include/Exclude Patterns for .next/types
There is a potential conflict: line 14 explicitly includes files in “.next/types/**/*.ts”, yet line 15 excludes the entire “.next/types” directory. This may result in the included files being unintentionally omitted from compilation. Please verify that this configuration meets your intent—if certain type declarations inside “.next/types” must be compiled, consider adjusting the exclude pattern (or using an explicit “files” array) so that the desired files are not inadvertently dropped.
Review the TS Config Include/Exclude for .next/types
After inspecting the configuration in apps/webservice/tsconfig.json (lines 14–15), it’s clear that the intent to include files under .next/types/**/*.ts is being conflicted by the exclusion of the entire .next/types folder. If you intend to compile type declarations in that directory, please adjust the exclusion—either by removing .next/types from the exclude array or by narrowing down the exclusion pattern—so that the intended files aren’t inadvertently dropped.
• File: apps/webservice/tsconfig.json (lines 14–15)
• Issue: The inclusion of .next/types/**/*.ts is overridden by excluding .next/types entirely.
• Recommendation: Revise the exclude pattern to ensure the necessary type declarations in .next/types are compiled.
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.
We use a standalone output, so we don't need this.
Does that makes sense.
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.
@zacharyblasczyk Yes, that makes perfect sense. With Next.js standalone output, you don't need to process the types in the .next/types directory since they're not relevant for your build. Thanks for the clarification!
✏️ Learnings added
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#363
File: apps/webservice/tsconfig.json:14-15
Timestamp: 2025-03-09T08:56:53.472Z
Learning: In the ctrlplane project, they use Next.js standalone output mode, which means they intentionally exclude `.next/types` in their tsconfig.json despite including `.next/types/**/*.ts` in their include pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Tested. |
Stop unnecessary component re-renders
Summary by CodeRabbit
New Features
Chores
@next/bundle-analyzerandnext, to their latest versions.