-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor: update non-exported types from interface to type #2113
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2113 +/- ##
=======================================
Coverage 97.11% 97.11%
=======================================
Files 112 112
Lines 2806 2806
Branches 763 763
=======================================
Hits 2725 2725
Misses 77 77
Partials 4 4 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
ad81cc7
to
393969e
Compare
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.
Mark any types that get exported as to be updated at the next major version.
Did your strategy change? I'm seeing some exported interfaces being converted to types, and some non exported interfaces being marked for next major version
Also, why the decision to delay some vs update others?
@@ -9,7 +9,7 @@ import Text from '../Text'; | |||
|
|||
import styles from './Card.module.css'; | |||
|
|||
export interface CardProps extends HTMLAttributes<HTMLElement> { | |||
export type CardProps = HTMLAttributes<HTMLElement> & { |
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.
should these ones instead be marked as todo for next major version?
@@ -6,7 +6,7 @@ import Icon from '../Icon'; | |||
import type { IconName } from '../Icon'; | |||
import styles from './FieldNote.module.css'; | |||
|
|||
export interface Props { | |||
export type FieldNoteProps = { |
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.
same comment for these
@timzchang Oh, I left out an important word here. exported externally (from src/index.ts). We have some internal exports for components and component tests to use, but the ones i wanted to postpone were those affecting the external API used by consumers. Changing those would be a breaking change technically, so waiting until the new year to batch together some breaking changes. |
Change TS convention to consistently use
type
instead of a mix oftype
andinterface
for components. Mark any types that get exported as to be updated at the next major version.NOTE: some changes come from tweaks to
prettier
config, so accepting thoseTest Plan: