-
Notifications
You must be signed in to change notification settings - Fork 535
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
Remove propTypes #1068
Remove propTypes #1068
Conversation
🦋 Changeset detectedLatest commit: 2dfc6f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/6XTLWcPSmLP39YaExNwCw2rtEyb2 |
if (!options.skipSx) { | ||
it('implements the sx prop', () => { |
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.
Do we have another way to make sure that all components are getting the correct system and sx props? Not sure how I feel about removing this without replacing it with something else 🤔
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 agree that we need a better way to test that system props are being correctly applied but I think that could be done in a separate PR. I don't think that these tests were adding that much value because they weren't testing if passing a system prop applied the correct styles.
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.
Approved but left one question!
Problem
Now that our components are all written in TypeScript, the developer experience benefits of using propTypes no longer outweighs the maintenance cost.
propTypes were helpful because they displayed console errors when a component was passed incorrect props. However, propTypes were hard to maintain, leading to many propTypes that were incorrect and unspecific (i.e.
PropTypes.object
).With TypeScript, developers now get errors directly in their editor or at build-time when a component is being used incorrectly. And we can be more confident in these errors because the TypeScript compiler enforces a higher level of type accuracy than our previous propTypes system.
Solution
This PR removes propTypes from every component and removes tests that checked for propTypes.