Skip to content

Conversation

@iansan5653
Copy link
Contributor

@iansan5653 iansan5653 commented Aug 26, 2024

After primer/react#4834, <Text size="..." /> and <Text weight="..." /> are valid uses of the Text component. But the no-unnecessary-components rule is currently only checking for sx and styled-system props, so these would get flagged.

This PR fixes that by adding a set of "allowed" props to each component. If any of these props are used, the component is considered valid. Also adds tests for this.

In addition, this PR improves the spread props typechecking: instead of just checking for allowed props, we extract all the property names, then check if each one is allowed. Because we don't have access to an array of styled-system property names, this is the only way to check for styled-system props in the spread props object -- something we weren't doing at all before. So this should make the rule slightly more accurate for a rare edge case, and also makes the code cleaner.

@iansan5653 iansan5653 requested a review from a team as a code owner August 26, 2024 14:00
@iansan5653 iansan5653 requested a review from jonrohan August 26, 2024 14:00
@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2024

⚠️ No Changeset found

Latest commit: 8c88de2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines +87 to 88
// Check if the spread type has a string index signature - this could hide an allowed property
if (typeChecker.getIndexTypeOfType(spreadType, IndexKind.String) !== undefined) return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched these checks around since theoretically getIndexOfType should be slightly faster than getPropertiesOfType so we can potentially short-circuit a little bit faster and save some time.

@iansan5653
Copy link
Contributor Author

This PR can safely skip changeset since it will go out in the same release as the PR that created the rule

@iansan5653 iansan5653 merged commit ab8269f into main Aug 26, 2024
@iansan5653 iansan5653 deleted the improve-no-unnecessary-components branch August 26, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants