🔧 (typescript) change moduleResolution to bundler #4163
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
MatissJanis
changed the title
WalkthroughThe pull request encompasses multiple changes across various packages and components, primarily focusing on TypeScript configuration and import statement reorganization. Key modifications include adding the In the Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/api/tsconfig.dist.json (1)
8-8: Consider documenting the different moduleResolution strategies.While the root config uses
bundler, this distribution config usesnode10. This difference is valid but should be documented to explain the reasoning.Consider adding a comment explaining why Node.js compatibility is needed for the distribution build:
"target": "ES2021", "module": "CommonJS", + // Using node10 resolution for Node.js compatibility in distribution builds "moduleResolution": "node10",🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
packages/desktop-client/src/components/settings/UI.tsx (1)
1-7: Consider type-safe CSS-in-JS alternatives for future iterations.While the current changes help with immediate type issues, consider these architectural improvements for the future:
- Evaluate CSS-in-JS alternatives that provide better TypeScript integration
- Consider creating a strongly-typed theme system
- Look into CSS modules or other solutions that provide compile-time type checking
This would help achieve better type safety while maintaining the benefits of the current system.
packages/desktop-client/src/style/styles.ts (1)
19-20: Document the temporary nature of this type definition.The use of
Record<string, any>aligns with the PR's stated interim approach. Consider adding a TODO comment to track the need for proper typing when moving away from@emotion/css.// eslint-disable-next-line @typescript-eslint/no-explicit-any +// TODO: Replace with proper typing when migrating away from @emotion/css export const styles: Record<string, any> = {packages/desktop-client/src/components/common/Button.tsx (1)
Line range hint
9-21: Consider adding type safety for specific CSS properties.While using a loose CSSProperties type works as an interim solution, consider gradually adding type safety for the most commonly used CSS properties in your components. This would help catch potential issues early while still maintaining flexibility.
Example approach:
type CommonButtonStyles = { backgroundColor?: string; color?: string; padding?: string; margin?: string; // Add other commonly used properties } & CSSProperties; // Update ButtonProps to use the more specific type type ButtonProps = HTMLProps<HTMLButtonElement> & { // ... other props style?: CommonButtonStyles; hoveredStyle?: CommonButtonStyles; activeStyle?: CommonButtonStyles; textStyle?: CommonButtonStyles; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4163.mdis excluded by!**/*.md
📒 Files selected for processing (25)
packages/api/tsconfig.dist.json(1 hunks)packages/crdt/tsconfig.dist.json(1 hunks)packages/desktop-client/src/components/Notes.tsx(1 hunks)packages/desktop-client/src/components/Titlebar.tsx(2 hunks)packages/desktop-client/src/components/common/Block.tsx(1 hunks)packages/desktop-client/src/components/common/Button.tsx(1 hunks)packages/desktop-client/src/components/common/InlineField.tsx(1 hunks)packages/desktop-client/src/components/common/Input.tsx(1 hunks)packages/desktop-client/src/components/common/InputWithContent.tsx(1 hunks)packages/desktop-client/src/components/common/Link.tsx(1 hunks)packages/desktop-client/src/components/common/Paragraph.tsx(1 hunks)packages/desktop-client/src/components/common/Text.tsx(1 hunks)packages/desktop-client/src/components/common/View.tsx(1 hunks)packages/desktop-client/src/components/forms.tsx(1 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx(2 hunks)packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/NetWorthGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/tableGraph/ReportTable.tsx(1 hunks)packages/desktop-client/src/components/select/DateSelect.tsx(1 hunks)packages/desktop-client/src/components/settings/UI.tsx(1 hunks)packages/desktop-client/src/components/sidebar/ItemContent.tsx(1 hunks)packages/desktop-client/src/components/table.tsx(1 hunks)packages/desktop-client/src/style/styles.ts(1 hunks)packages/desktop-electron/tsconfig.dist.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/desktop-client/src/components/common/View.tsx
- packages/desktop-client/src/components/common/InputWithContent.tsx
- packages/desktop-client/src/components/table.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetMonthMenuModal.tsx
- packages/desktop-client/src/components/reports/graphs/NetWorthGraph.tsx
- packages/desktop-client/src/components/select/DateSelect.tsx
- packages/desktop-client/src/components/Titlebar.tsx
- packages/desktop-client/src/components/modals/TrackingBudgetMonthMenuModal.tsx
- packages/desktop-client/src/components/forms.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/crdt/tsconfig.dist.json
[error] 7-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
packages/api/tsconfig.dist.json
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
packages/desktop-electron/tsconfig.dist.json
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🔇 Additional comments (19)
packages/desktop-client/src/components/common/Link.tsx (1)
11-11: LGTM! Import change aligns with project direction.The change to import
CSSPropertiesfrom the local style module aligns with the PR objectives of centralizing CSS types and preparing for component library extraction.Let's verify that the local style module's
CSSPropertiestype supports the custom CSS properties used in this file:✅ Verification successful
Verified: CSSProperties import change is type-safe
The local
CSSPropertiestype (Record<string, any>) fully supports all CSS properties used in the Link component, including custom selectors and data attributes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the local CSSProperties type supports custom CSS properties # Find the style module that exports CSSProperties fd -e ts -e tsx "style\.(ts|tsx)$" packages/desktop-client/src # Check the CSSProperties type definition ast-grep --pattern 'type CSSProperties = $_'Length of output: 3468
packages/desktop-client/src/components/common/Text.tsx (2)
Line range hint
1-28: LGTM! Well-structured component with proper TypeScript typing.The component is well-implemented with:
- Proper use of forwardRef
- Clean prop types definition
- Correct emotion CSS handling
- Good TypeScript practices
10-10: Verify the local style module's type compatibility.The switch from React's
CSSPropertiesto a local style module aligns with the PR objectives. However, we should verify that the local type definition includes all necessary CSS properties used in the codebase.Let's check the local style module implementation and its usage:
✅ Verification successful
Local CSSProperties type is compatible with existing usage.
The local style module implements CSSProperties as a flexible type that maintains compatibility with emotion/css and existing component styles. The implementation is intentionally loose-typed to match React's CSSProperties behavior, making this a safe migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the local style module implementation and its usage across components # Check the style module implementation echo "Checking style module implementation:" cat packages/desktop-client/src/style.ts || cat packages/desktop-client/src/style/index.ts # Find all files using CSSProperties from the local style module echo -e "\nChecking usage of local CSSProperties:" rg -l "CSSProperties.*from '../../style'" packages/desktop-client/src/components/Length of output: 1375
Script:
#!/bin/bash # Check the CSSProperties type definition and related imports echo "Checking styles.ts content:" cat packages/desktop-client/src/style/styles.ts echo -e "\nChecking for CSSProperties type usage in style directory:" rg "CSSProperties" packages/desktop-client/src/style/ -A 3Length of output: 6546
packages/desktop-client/src/components/sidebar/ItemContent.tsx (3)
12-12: LGTM! Type change improves type safety.The change from
CSSPropertiestoComponentProps<typeof View>['style']provides better type safety by ensuring the style prop matches exactly what the View component accepts.
Line range hint
19-48: LGTM! Component implementation is solid.The component's implementation properly handles both clickable and link states, with consistent style application and good accessibility attributes (role, tabIndex).
4-4: Verify the local style module implementation.The change to import
CSSPropertiesfrom a local style module aligns with the PR objectives. However, we should verify that the local style module properly defines all necessary CSS properties.Also applies to: 7-7
✅ Verification successful
Local CSSProperties type implementation is verified and safe
The change to use the local CSSProperties type is verified to be safe as it maintains the same type flexibility as React's CSSProperties while supporting the PR's goal of component library extraction. The type is already widely used across the codebase (100+ files) and properly supports all CSS properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of the local style module # and its usage across the codebase # Check the local style module implementation cat packages/desktop-client/src/style.ts || echo "Style module not found at expected location" # Check for other files using the same import pattern rg "from '../../style'" packages/desktop-client/src/componentsLength of output: 15908
Script:
#!/bin/bash # Search for style file with different extensions and check tsconfig fd -e ts -e tsx -e js -e jsx "style" packages/desktop-client/src # Check tsconfig for path aliases and the actual style file content cat packages/desktop-client/tsconfig.json || echo "tsconfig.json not found" # Search for CSSProperties type definition rg -l "CSSProperties" packages/desktop-client/srcLength of output: 8571
Script:
#!/bin/bash # Check the implementation of CSSProperties in the style module cat packages/desktop-client/src/style/styles.ts # Verify if there are any other style-related type definitions rg -g "*.ts" -g "*.tsx" "type CSSProperties" packages/desktop-client/src/style/Length of output: 5749
tsconfig.json (1)
28-29: Verify compatibility with existing build tools and bundlers.The change from
Node10tobundlermoduleResolution is aligned with the PR objectives for component library extraction. However, this change might affect how modules are resolved across the project.Run the following to verify module resolution compatibility:
✅ Verification successful
Module resolution change is safe to proceed
The change from
"Node10"to"bundler"moduleResolution is compatible with the codebase. Analysis shows:
- No bare module imports that would be affected by the resolution strategy change
- Modern import patterns throughout the codebase
- No reliance on Node10-specific module resolution features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential module resolution issues # Look for relative imports that might be affected rg -g '*.{ts,tsx}' -l '\.\.' packages/ # Check for package.json files that might need updates fd -g 'package.json' -x cat {} \; -x echo '---'Length of output: 68228
Script:
#!/bin/bash # Check for package.json files with "exports" field fd -g 'package.json' -x jq -r 'select(.exports != null) | "\(.name): \(.exports)"' {} \; # Look for potential problematic import patterns rg -g '*.{ts,tsx}' '(from|import) ["\'`](?!\.|@|[a-zA-Z]+:)' --no-filename --no-line-number | sort -u # Check TypeScript version in lockfile rg '"typescript"' yarn.lock -A 2Length of output: 245
Script:
#!/bin/bash # Check bare imports that might be affected by moduleResolution change rg -g '*.{ts,tsx}' '^import .+ from "[^\./@]' --no-filename --no-line-number | sort -u # Check TypeScript version and related dependencies rg -g 'yarn.lock' -A 3 '"typescript@\^' # Look for potential module resolution comments/directives rg -g '*.{ts,tsx}' '// @ts-ignore|// @ts-expect-error|moduleResolution|paths' --no-filename --no-line-number | sort -uLength of output: 4178
packages/crdt/tsconfig.dist.json (1)
8-8: LGTM! Consistent with other distribution configs.The moduleResolution strategy matches other distribution configs, maintaining consistency across packages.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
packages/desktop-electron/tsconfig.dist.json (1)
8-8: LGTM! Consistent configuration approach.The moduleResolution strategy aligns with other distribution configs, and the formatting is cleaned up.
Also applies to: 11-11
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
packages/desktop-client/src/components/common/Block.tsx (1)
1-5: Verify type compatibility with emotion's css function.The change from React's CSSProperties to local style definition needs verification to ensure all CSS properties used with emotion's css function are still valid.
✅ Verification successful
Type compatibility verified successfully
The change from React's CSSProperties to local CSSProperties is safe. The local type definition (
Record<string, any>) is fully compatible with emotion's css function and provides sufficient type coverage for all CSS properties.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all CSS properties used with emotion's css function in Block components rg -U "css\(\$?{[^}]*}\)" packages/desktop-client/src/components/common/Block.tsxLength of output: 189
Script:
#!/bin/bash # Search for emotion's css function usage with proper escaping rg -U "css\(.*\)" packages/desktop-client/src/components/common/Block.tsx # Find style prop type definition and usage ast-grep --pattern 'interface $_ { $$$ style?: $_; $$$ }' # Look for style object assignments rg "style[=:]" packages/desktop-client/src/components/common/Block.tsxLength of output: 294
Script:
#!/bin/bash # Find Block component interface definition ast-grep --pattern 'interface BlockProps { $$$ }' # Check local CSSProperties definition cat packages/desktop-client/src/style.ts # Find any other usages of CSSProperties in the file rg "CSSProperties" packages/desktop-client/src/components/common/Block.tsxLength of output: 315
Script:
#!/bin/bash # Find the style file containing CSSProperties definition fd style.ts # Backup search for CSSProperties definition rg -g "*.ts" -g "*.tsx" "export.*CSSProperties"Length of output: 517
packages/desktop-client/src/components/common/Paragraph.tsx (1)
1-5: Verify compatibility of hardcoded CSS properties.The component uses hardcoded CSS properties (marginBottom, lineHeight). Ensure these properties are properly typed in the local CSSProperties definition.
✅ Verification successful
CSS properties are compatible with type definition
The hardcoded CSS properties (marginBottom: 15, lineHeight: '1.5em') are compatible with the local CSSProperties type, which is defined as
Record<string, any>and accepts any property name with any value type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded CSS property usage in Paragraph components rg -U "css\(\[[^\]]*\]\)" packages/desktop-client/src/components/common/Paragraph.tsxLength of output: 228
Script:
#!/bin/bash # Find and show CSSProperties definition ast-grep --pattern 'type CSSProperties = $_'Length of output: 3403
packages/desktop-client/src/components/common/InlineField.tsx (1)
1-5: Verify type safety of nested style objects.The component uses nested style objects with specific CSS properties. Ensure the local CSSProperties type properly handles these nested cases.
✅ Verification successful
Style implementation is type-safe
The component correctly uses CSSProperties for both the main style prop and nested style object. The implementation safely merges styles using emotion's css() function and only uses basic CSS properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for nested style object usage rg -U "style={{[^}]*}}" packages/desktop-client/src/components/common/InlineField.tsxLength of output: 192
Script:
#!/bin/bash # Search for style prop usage with better pattern rg "style=\{" packages/desktop-client/src/components/common/InlineField.tsx # Look for CSSProperties type usage rg "CSSProperties" packages/desktop-client/src/components/common/InlineField.tsx # Get the full component implementation cat packages/desktop-client/src/components/common/InlineField.tsxLength of output: 1140
packages/desktop-client/src/components/settings/UI.tsx (1)
1-1: Verify theme and media query compatibility.The component uses theme values and media queries extensively. Ensure the local CSSProperties type properly handles:
- Theme color values
- Media query expressions
- Dynamic style composition with theme values
Also applies to: 7-7
✅ Verification successful
Theme and media query usage verified as compatible ✅
The component's usage of theme values, media queries, and style composition follows standard React patterns and is fully compatible with CSSProperties type. All style properties, including theme colors and media queries, are properly typed and supported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for theme usage in styles rg -U "theme\.[a-zA-Z]+" packages/desktop-client/src/components/settings/UI.tsx # Search for media query usage rg -U "@media" packages/desktop-client/src/components/settings/UI.tsxLength of output: 356
Script:
#!/bin/bash # Check theme type definitions ast-grep --pattern 'type Theme = { $$$ }' # Check token definitions ast-grep --pattern 'const tokens = { $$$ }' # Check style composition patterns rg -U "style=\{[^}]+\}" packages/desktop-client/src/components/settings/UI.tsxLength of output: 635
packages/desktop-client/src/style/styles.ts (1)
10-11: Consider using a stricter type definition for CSSProperties.While using
Record<string, any>provides flexibility, it removes all type safety. This could lead to runtime errors that TypeScript won't catch.Consider one of these alternatives:
- Extend React's CSSProperties:
type CSSProperties = React.CSSProperties & Record<string, unknown>- Use a CSS-in-JS type library:
@emotion/reactprovides its own types- Create a custom type with only the additional properties you need
packages/desktop-client/src/components/common/Input.tsx (1)
12-12: LGTM!The import change aligns with the project-wide effort to centralize CSS type definitions.
packages/desktop-client/src/components/Notes.tsx (1)
2-2: LGTM!The import changes are consistent with the project-wide migration to use the local CSS type definitions.
Also applies to: 9-9
packages/desktop-client/src/components/reports/graphs/tableGraph/ReportTable.tsx (1)
16-16: Consider adding runtime type checks for critical style properties.Given that this file heavily relies on CSS properties in multiple type definitions (ReportTableProps, renderTotalsProps, renderRowProps), consider adding runtime validation for critical style properties to catch potential type-related issues early.
Example approach:
function validateStyles(styles: CSSProperties, requiredProps: string[]): void { requiredProps.forEach(prop => { if (styles && prop in styles && typeof styles[prop] === 'undefined') { console.warn(`Missing required style property: ${prop}`); } }); }packages/desktop-client/src/components/common/Button.tsx (2)
1-1: Verify type compatibility of local CSSProperties implementation.The migration from React.CSSProperties to a local implementation aligns with the PR objectives. However, we should ensure that the local CSSProperties type (which is likely more permissive as mentioned in the PR description) doesn't mask potential type issues.
Let's verify the implementation of the local CSSProperties type:
Also applies to: 6-6
✅ Verification successful
Local CSSProperties implementation confirmed as intentionally permissive
The local CSSProperties type is implemented as
Record<string, any>, which aligns with the PR's objective of moving away from React's strict CSS property typing. While this trades type safety for flexibility, it appears to be an intentional design decision.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of CSSProperties in the style module rg -A 5 "type CSSProperties" packages/desktop-client/src/styleLength of output: 509
1-6: Ensure consistent CSSProperties migration across components.As Button is a foundational component, verify that all consuming components are aligned with this change from React.CSSProperties to the local implementation.
Let's check for any remaining React.CSSProperties usage:
✅ Verification successful
Migration to local CSSProperties is complete and consistent
All components in the codebase are correctly using the local CSSProperties type from the style module. No instances of React.CSSProperties remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining React.CSSProperties usage rg "React\.CSSProperties" packages/desktop-client/src/ # Search for CSSProperties imports to verify consistency rg "import.*CSSProperties.*from.*style" packages/desktop-client/src/Length of output: 2484
Changed
moduleResolutiontobundler. This will allow us to extract the common components into a separate component library that can then be re-used in plugins.The change of
moduleResolutionexposed some issues with our current usage ofReact.CSSProperties. Namely: we are using properties that do not exist in this type, but there were no warnings about this.There are a couple of potential solutions. Some described here: https://mattrossman.com/2024/01/19/css-variables-with-react-and-typescript
However, I think in the end we will need to bite the bullet and move away from
@emotion/csswhich will also require many type changes. So it will be much easier to do this if we fully control theCSSPropertiestype. Hence why I extracted it into our codebase. It's definitely not perfect (as it currently is super loose).. but it's a starting point...ps. - I don't plan to continue working on the CSS tangent. Focusing now on the shared component library and helping the plugins initiative.