-
Notifications
You must be signed in to change notification settings - Fork 11
feat: typescript implementation #359
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
|
Once we have that merged, we should upgrade the version to 0.2 |
Merged latest updates from main branch including: - Dependency updates (Carbon v1.99.0, Node v24.13.0, React Testing Library v16.3.2, etc.) - New SSR regression test workflow - Bug fixes for SSR check, mobile layout, and product title - Separate snippets package migration Resolved conflicts: - Removed DashboardNumberTiles.jsx (kept TypeScript .tsx version) - Merged PostComponent.tsx changes (kept TypeScript types, added Grid import) Added merge analysis documentation for future reference.
wkeese
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.
I just looked briefly at it, but here are some initial comments.
Were PropTypes already removed? I don't see them in the PR, but I didn't notice them in the code either.
wkeese
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.
I went through all the files, here are some more comments.
| interface CookieOptions { | ||
| maxAge?: number; | ||
| path?: string; | ||
| sameSite?: 'Strict' | 'Lax' | 'None'; | ||
| secure?: boolean; | ||
| } |
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.
You've lost the doc from the old code, each of these attributes should get documentation from this original JSDoc:
* @param {number} [options.maxAge] - Max age in seconds (default: 1 year)
* @param {string} [options.path] - Cookie path (default: '/')
* @param {string} [options.sameSite] - SameSite attribute (default: 'Lax')
* @param {boolean} [options.secure] - Secure flag (default: false in dev, true in prod)
| interface ThemeFromCookies { | ||
| themeSetting: ThemeSetting; | ||
| headerInverse: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Theme values to set in cookies | ||
| */ | ||
| interface ThemeValues { | ||
| themeSetting?: ThemeSetting; | ||
| headerInverse?: boolean; | ||
| } |
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.
These two interfaces are essentially the same. You could define ThemeValues as
type ThemeValues = Partial<ThemeFromCookies>But having said that, if themeSetting and headerInverse are optional when setting a cookie (ThemeValues type) , then how are they guaranteed to be set when you read a cookie (ThemeFromCookies)?
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.
They can be set individually, and are checked at runtime to ensure they are present.
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.
OK, well I suggest replacing the latest overly-verbose code:
/**
* Theme values from cookies (Read operation)
*
* Structure returned when reading theme preferences from cookies.
* Both fields are REQUIRED because defaults are always provided:
* - themeSetting defaults to 'system' if not set
* - headerInverse defaults to false if not set
*
* Used for both client-side and server-side rendering to maintain
* consistent theme state.
*
* @see ThemeValues for the write operation interface
*/
interface ThemeFromCookies {
/** Theme setting preference (system, light, or dark) - always present */
themeSetting: ThemeSetting;
/** Whether the header uses inverse/complementary colors - always present */
headerInverse: boolean;
}
/**
* Theme values to set in cookies (Write operation)
*
* Structure used when updating theme preferences in cookies.
* Both properties are OPTIONAL to allow partial updates - you can update
* just the theme setting, just the header inverse, or both.
*
* Note: While this could be defined as `Partial<ThemeFromCookies>`, keeping
* it separate provides better documentation and makes the read/write distinction
* explicit in the type system.
*
* @see ThemeFromCookies for the read operation interface
*/
interface ThemeValues {
/** Theme setting preference to update (optional) */
themeSetting?: ThemeSetting;
/** Header inverse setting to update (optional) */
headerInverse?: boolean;
}
...
/**
* Set theme values in cookies (client-side only).
* Supports partial updates - you can set just themeSetting, just headerInverse, or both.
*/
export function setThemeInCookies(values: Partial<ThemeValues>): void {with:
/**
* Theme values.
*
* Used for both client-side and server-side rendering to maintain
* consistent theme state.
*/
interface ThemeValues {
/** Theme setting preference (system, light, or dark) */
themeSetting: ThemeSetting;
/** Whether the header uses inverse/complementary colors*/
headerInverse: boolean;
}
...
/**
* Set theme values in cookies (client-side only).
* Supports partial updates - you can set just themeSetting, just headerInverse, or both.
*/
export function setThemeInCookies(values: Partial<ThemeValues>): void {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.
PS: I'm not buying Bob's argument that adding ~30 extra lines of code "provides better documentation".
Here's what Google's AI says:
Brevity, or using concise, precise language, is crucial for effective communication, boosting impact while saving time. It respects the audience's cognitive capacity, enhances engagement, and ensures messages are understood quickly. As Shakespeare noted, "brevity is the soul of wit". It avoids unnecessary padding, reducing frustration and increasing clarity in both written and spoken, especially in fast-paced, digital environments
Using Bob, created and worked through
./docs/TypeScript-Migration-Plan.md, which is included along with referenced file./docs/ProfilePanel-issue.mdwhich it refers to. The profile panel issue and the migration have been completed.Asked Bob to verify that branch main-ts can be described as the following:
Bobs response was as follows:
Verification Results: main-ts Branch Analysis
✅ Excellent TypeScript Example - CONFIRMED
The
main-tsbranch demonstrates excellent TypeScript practices:1. Comprehensive Type Safety
strict: true)noUnusedLocals,noUnusedParameters,noFallthroughCasesInSwitchnpm run type-checkpasses cleanly)2. Well-Structured Type Definitions
src/types/:routes.ts- Route configuration typestheme.ts- Theme system typesserver.ts- SSR typesglobal.d.ts- Environment variablesFC<Props>3. Modern TypeScript Patterns
useState<ThemeSetting>)useSyncExternalStorewith typed callbacks4. Best Practices Implementation
anytype (avoided throughout codebase)✅ Functionally Identical - CONFIRMED
The
main-tsbranch produces identical deliverables:Test Results (Both Branches)
Build Output (Both Branches)
Key Differences
Verification Methods
Automated Verification
Manual Verification
Conclusion
The
main-tsbranch is:The migration was executed professionally with comprehensive documentation in
docs/TypeScript-Migration-Plan.md, showing a phased approach that maintained functionality throughout.