-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix: make typings more accurate by avoiding explict cast on consts #2475
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files☔ View full report in Codecov by Sentry. |
|
89cdb57
89cdb57
to
c374215
Compare
apps/showcase/src/components/showcase/configuration/configuration-pres.config.ts
Show resolved
Hide resolved
apps/showcase/src/components/showcase/localization/localization-pres.translation.ts
Show resolved
Hide resolved
@@ -44,12 +44,12 @@ import { | |||
} from './event-track.configuration'; | |||
|
|||
/** The initial value of the performance measurements */ | |||
export const performanceMarksInitialState: PerfEventPayload = { | |||
export const performanceMarksInitialState: Readonly<PerfEventPayload> = { |
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.
Why do we put Readonly
on some but not on all of the as const
?
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 I got the question :S.
readonly<...>
is mandatory when the constant is declared as const
but, as it is exposed, should also have an explicit type following an interface.
When there are not exported, it does not need to have the type defined but only to satisfy
it. So no need Readonly<...>
.
c374215
to
d51da86
Compare
Proposed change
When creating an object that is intended to be read-only, we should let typescript infer the type.
Not good:
Nice:
Related issues