-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add strict type checks for components' props #1290
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1290 +/- ##
==========================================
- Coverage 72.36% 72.32% -0.04%
==========================================
Files 758 759 +1
Lines 5684 5692 +8
Branches 1663 1687 +24
==========================================
+ Hits 4113 4117 +4
- Misses 1565 1569 +4
Partials 6 6
Continue to review full report at Codecov.
|
docs/src/components/ComponentDoc/ComponentControls/ComponentControls.tsx
Outdated
Show resolved
Hide resolved
docs/src/components/ComponentDoc/ComponentProps/ComponentProps.tsx
Outdated
Show resolved
Hide resolved
This doesn't work for VSCode at all for |
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 don't want to block these changes because I see benefits from them.
However I am thinking that changes in run-time code to satisfy TypeScript is something evil 💣
would want to avoid them as well, but for now, given that we have class components and there the only option are generics, as well as that it is impossible to introduce function overload semantics with regular TS types, I don't see any better approach there - and noop runtime tricks is the only way to deal with that. Also, in case if solution will be found (introduced by TS), it will be quite easy for us to adopt it, as this type is applied consistently to all components, so we might be able to introduce necessary fixes to it. |
{element} | ||
</Provider> | ||
) | ||
return <Provider theme={newTheme}>{element}</Provider> |
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.
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.
got it - the thing is that this variables
prop was missing from the Provider
's API, and that has led me to confusion. Will address it
Fixes #1263.
BREAKING CHANGES
Restricted type checks introduced by this PR may introduce breaking changes on the client side - however, most certain this will be an indication of problem in client's logic.
However, to mitigate potential migration problems, the following fallback option was introduced, to loose type checks to the same state they were working before.
Fallback option to loose type checks
In the exceptional cases, to mitigate migration risks on the client side, the fallback type was introduced as part of this PR. This type corresponds to the one that clients originally had for Stardust components before this update.
Description
Provides strict type checks for Stardust components' props.
Type checks introduced
Examples of type checks introduced are provided with
Button
component taken, which supportscircular: boolean
propformEncType: string
prop from the interface of intrinsicbutton
element, which is rendered by default asButton
's element type.No 'as' prop defined (regular case)
With 'as' prop defined
Type checks for
as
case were intentionally loosened, to avoidheap out-of-memory
problem of TS compiler (more on that below). However, even with that provided type system was sufficient enough to capture all the same type problems in Stardust codebase that would be discovered by the strictest type-check solution, and generally is expected to prevent most of type problems on the client side.Stardust problems discovered (just files list, for brevity)
create-react-app
test projectAdditional cleanup
...WithDefault
types were removed from:Divider
,ItemLayout
,Layout
as
prop by design were updated:Provider
,Popup
,Portal
,PortalInner
Main type files changed
Notes on the approach
The following things were taken into account in the solution provided.
Avoid 'heap out of memory' problem from TS Compiler
In order to prevent cause of this TS compiler's problem (which repros even for the latest to date, v3.4.5 of TS compiler), it was necessary to loose type checks for the cases when
as
prop is explicitly specified by client's code.However, even with that provided type system was sufficient enough to capture all the same type problems in Stardust codebase that would be discovered by the strictest type-check solution, and generally is expected to prevent majority of type problems on the client side.
Introduced types don't cause 'heap out of memory' problem for TS compiler - as component type overloads do not cause deep recursion like the following one, while processed by TS engine:
Explicit prop type declarations in component types
Exported components are wrapped in
withSafeTypeForAs
type that requires type of component props to be explicitly provided in generic args:While component prop types could be inferred, this approach was taken because of its huge performance benefits introduced to TS Server, as it helps to avoid unnecessary inference and significantly reduce load on TS server. With that, we are making things explicit only once while exporting component, and reap performance benefits of rapid TS checks every time it is used.
These are comparison metrics between 'solution with inference' and 'solution with no inference'
yarn build
timeNon-related problems spotted
Embed
has description for GIF (Wrong description for Embed component #1330)