-
Notifications
You must be signed in to change notification settings - Fork 5k
fix(twenty-front): fix tsconfig to properly typecheck all files with tsgo #17380
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: main
Are you sure you want to change the base?
Conversation
…tsgo The previous tsconfig setup used project references with `files: []`, which caused `tsgo -p tsconfig.json` to check nothing (tsgo requires `-b` flag for project references, but the configs weren't set up for composite mode). Changes: - Simplified tsconfig architecture from 4 files to 2: - tsconfig.json: all files (dev, tests, stories) for typecheck/IDE/lint - tsconfig.build.json: production files only (excludes tests/stories) - Removed redundant tsconfig.dev.json, tsconfig.spec.json, tsconfig.storybook.json - Updated jest.config.mjs, eslint.config.mjs, vite.config.ts to use new configs - Made applicationId optional in FieldMetadataItem and ObjectMetadataItem (new schema field not needed in mock data) - Added missing navigationMenuItem translation - Added objectLabelSingular to Search GraphQL query (new required field) - Fixed sortMorphItems.test.ts to include objectLabelSingular This fix enables proper type checking and revealed several pre-existing type errors that were previously hidden.
88e79ea to
5f20276
Compare
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:24842 This environment will automatically shut down when the PR is closed or after 5 hours. |
4412ec7 to
77a9411
Compare
…tsgo The previous tsconfig setup used project references with `files: []`, which caused `tsgo -p tsconfig.json` to check nothing (tsgo requires the configs to include files directly, not through project references). ## Changes by package ### twenty-front - Simplified from 4 configs to 2: tsconfig.json (all files) + tsconfig.build.json (prod) - Removed: tsconfig.dev.json, tsconfig.spec.json, tsconfig.storybook.json - Updated jest.config.mjs, eslint.config.mjs, vite.config.ts references - Made applicationId optional in FieldMetadataItem and ObjectMetadataItem - Added missing navigationMenuItem translation - Added objectLabelSingular to Search GraphQL query ### twenty-ui - Simplified from 5 configs to 2: tsconfig.json (all files) + tsconfig.lib.json (build) - Removed: tsconfig.dev.json, tsconfig.spec.json, tsconfig.storybook.json - Removed baseUrl (not supported by tsgo) - Updated vite.config.ts and .storybook/main.ts references ### twenty-shared - Simplified from 3 configs to 2: tsconfig.json (all files) + tsconfig.lib.json (build) - Removed: tsconfig.spec.json - Removed baseUrl (not supported by tsgo) ### twenty-sdk - Simplified from 3 configs to 2: tsconfig.json (all files) + tsconfig.lib.json (build) - Removed: tsconfig.spec.json ### twenty-emails - Simplified to single tsconfig.json (all files) ### twenty-eslint-rules - Simplified from 3 configs to 1: tsconfig.json (all files) - Removed: tsconfig.lint.json, tsconfig.spec.json - Updated jest.config.mjs reference ### create-twenty-app - Simplified from 3 configs to 2: tsconfig.json (all files) + tsconfig.lib.json (build) - Removed: tsconfig.spec.json ## Note on pre-existing type errors Proper type checking now reveals ~77 pre-existing errors in twenty-front and ~1100 in twenty-server that were previously hidden. These are primarily: - Icon prop type mismatches (twenty-ui icons have required props that are optional in IconComponentProps) - Type inference issues with Object.keys() returning string | number | symbol These should be fixed in follow-up PRs.
77a9411 to
805f345
Compare
f7c2a85 to
5076979
Compare
…ases - Fix twenty-ui icon components to use @ui/ path alias instead of package export path - Fix twenty-ui icon component types to accept full IconComponentProps - Fix twenty-emails vite config to resolve src/ path alias
JSON imports have string types instead of enum values, so the normalize function needs to accept a looser input type.
Satisfy the twenty/component-props-naming lint rule by adding type aliases (e.g., IconGoogleProps = IconComponentProps) while maintaining type compatibility with IconComponent.
…paths The tsconfig.json paths are relative to the package directory, so the jest prefix should be '<rootDir>/' instead of '<rootDir>/../../'.
The tsconfig has both @/* and src/* paths, so both need to be added to the vite resolve.alias config.
Greptile OverviewGreptile SummaryThis PR fixes TypeScript typechecking in Key Changes:
Type Fixes (pre-existing errors now caught):
Similar pattern applied to:
The changes are well-structured and enable proper typechecking that was previously not happening, revealing legitimate type issues that have been fixed. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant TSC as TypeScript Compiler
participant Jest as Jest
participant ESLint as ESLint
participant Vite as Vite Build
Note over Dev,Vite: Before: Project References Setup
Dev->>TSC: tsc -p tsconfig.json
TSC->>TSC: Check files: [] (empty)
TSC-->>Dev: No files checked ❌
Note over Dev,Vite: After: Direct Includes Setup
Dev->>TSC: tsc -p tsconfig.json
TSC->>TSC: Check all src/**, tests, stories
TSC->>TSC: Validate types (reveals errors)
TSC-->>Dev: Type errors found
Dev->>Dev: Fix applicationId (make optional)
Dev->>Dev: Add objectLabelSingular to query
Dev->>Dev: Update test mocks
Dev->>TSC: tsc -p tsconfig.json
TSC->>TSC: Typecheck all files
TSC-->>Dev: ✓ All types valid
Dev->>Jest: npm test
Jest->>Jest: Load tsconfig.json (not tsconfig.spec.json)
Jest->>Jest: Run tests with correct paths
Jest-->>Dev: ✓ Tests pass
Dev->>ESLint: npm run lint
ESLint->>ESLint: Load tsconfig.json (not 3 separate configs)
ESLint-->>Dev: ✓ Lint pass
Dev->>Vite: npm run build
Vite->>Vite: Load tsconfig.build.json
Vite->>Vite: Exclude tests/stories
Vite-->>Dev: ✓ Production build
|
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.
No files reviewed, no comments
| export type ButtonGroupProps = Pick< | ||
| ButtonProps, | ||
| 'variant' | 'size' | 'accent' | ||
| export type ButtonGroupProps = Partial< |
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.
Not sure why this would be needed? Todo check after it's merged since we need to merge quickly
.../twenty-front/src/modules/object-record/record-field/ui/utils/computeDraftValueFromString.ts
Outdated
Show resolved
Hide resolved
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.
2 issues found across 90 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-ui/src/display/icon/components/IconLock.tsx">
<violation number="1" location="packages/twenty-ui/src/display/icon/components/IconLock.tsx:8">
P2: IconLockCustom now accepts all IconComponentProps but only uses size, so className/style/color/stroke props are dropped. Forward the remaining props to the SVG so the expanded API actually works.</violation>
</file>
<file name="packages/twenty-ui/src/display/icon/components/IconTwentyStar.tsx">
<violation number="1" location="packages/twenty-ui/src/display/icon/components/IconTwentyStar.tsx:8">
P2: IconTwentyStar now accepts full IconComponentProps but drops className/style/color (and any future props) instead of forwarding them to the SVG. This makes the public API misleading and prevents styling/coloring via props.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export const IconLockCustom = ({ size }: IconLockCustomProps) => { | ||
| const theme = useTheme(); | ||
| const size = props.size ?? theme.icon.size.lg; | ||
| const iconSize = size ?? theme.icon.size.lg; | ||
|
|
||
| return <IconLockRaw height={size} width={size} />; | ||
| return <IconLockRaw height={iconSize} width={iconSize} />; |
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.
P2: IconLockCustom now accepts all IconComponentProps but only uses size, so className/style/color/stroke props are dropped. Forward the remaining props to the SVG so the expanded API actually works.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-ui/src/display/icon/components/IconLock.tsx, line 8:
<comment>IconLockCustom now accepts all IconComponentProps but only uses size, so className/style/color/stroke props are dropped. Forward the remaining props to the SVG so the expanded API actually works.</comment>
<file context>
@@ -3,11 +3,11 @@ import { useTheme } from '@emotion/react';
+type IconLockCustomProps = IconComponentProps;
-export const IconLockCustom = (props: IconLockCustomProps) => {
+export const IconLockCustom = ({ size }: IconLockCustomProps) => {
const theme = useTheme();
- const size = props.size ?? theme.icon.size.lg;
</file context>
| export const IconLockCustom = ({ size }: IconLockCustomProps) => { | |
| const theme = useTheme(); | |
| const size = props.size ?? theme.icon.size.lg; | |
| const iconSize = size ?? theme.icon.size.lg; | |
| return <IconLockRaw height={size} width={size} />; | |
| return <IconLockRaw height={iconSize} width={iconSize} />; | |
| export const IconLockCustom = ({ size, ...props }: IconLockCustomProps) => { | |
| const theme = useTheme(); | |
| const iconSize = size ?? theme.icon.size.lg; | |
| return <IconLockRaw height={iconSize} width={iconSize} {...props} />; |
packages/twenty-ui/src/display/icon/components/IconTwentyStar.tsx
Outdated
Show resolved
Hide resolved
…tValueFromString Instead of using 'as unknown' cast, properly initialize all required FieldAddressDraftValue fields with null values.
.../twenty-front/src/modules/object-record/record-field/ui/utils/computeDraftValueFromString.ts
Show resolved
Hide resolved
Revert unnecessary changes to icon component files. The original Pick<IconComponentProps, 'size'> types were correct and should not expose unused props.
The Partial wrapper was unnecessary since IconComponentProps properties are already optional.
Summary
This PR fixes the
tsconfigsetup intwenty-frontso thattsgo -p tsconfig.jsonproperly type-checks all files.Root Cause
The previous setup used TypeScript project references with
files: []in the maintsconfig.json. When runningtsgo -p tsconfig.json, this checks nothing becausetsgorequires the-b(build) flag for project references, but the configs weren't set up for composite mode.Changes
Simplified tsconfig architecture (4 files → 2):
tsconfig.json- All files (dev, tests, stories) for typecheck/IDE/linttsconfig.build.json- Production files only (excludes tests/stories)Removed redundant configs:
tsconfig.dev.jsontsconfig.spec.jsontsconfig.storybook.jsonUpdated references:
jest.config.mjs→ usestsconfig.jsoneslint.config.mjs→ usestsconfig.jsonvite.config.ts→ usestsconfig.jsonfor devType fixes (pre-existing errors revealed by proper typechecking):
applicationIdoptional inFieldMetadataItemandObjectMetadataItemnavigationMenuItemtranslationobjectLabelSingularto Search GraphQL querysortMorphItems.test.tsmock dataTest plan
npx nx typecheck twenty-front- should passnpx nx lint twenty-front- should worknpx nx test twenty-front- should worknpx nx build twenty-front- should work