Skip to content
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

Lint against strings without wrapping <Text> #3398

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 4, 2024

This adds a lint rule enforcing that JSX text has an immediate <Text> parent. There are a few nuances:

  • It skips over <Trans> when looking upwards for <Text>. So <Text><Trans>text</Trans></Text> is OK.
  • Aside from <Trans>, the parent has to be immediate. I.e. <Foo><Text>text</Text></Foo> is OK but <Text><Foo>text</Foo></Text> is not.
  • It treats any component ending with -Text as a valid parent. E.g. <FooText>text</FooText> is OK. This is useful for the design system.
    • Our components don't currently follow this scheme so I also added a list of exceptions. I do want to narrow them down though. The advantage of the suffix is that we'll be able to lint for it too — i.e. we'd be able to enforce that a component named FooText must return Text. This would close the remaining loophole.
  • It treats any prop called text (or ending with Text) as an implied guarantee it's wrapped. So <Foo bla={<Trans>hello</Trans>} /> is not OK but <Foo blaText={<Trans>hello</Trans>} /> is OK.
    • Our components don't currently follow this scheme so I also added a list of exceptions.
  • Note that we don't validate literals. So <Foo>{'text'}</Foo> will pass despite being not OK. Only JSX text is validated. This leaves much less space for false positives, and is probably fine because we have to wrap stuff in <Trans> anyway.

This gets us pretty far.

Examples

Raw text in <View>

Screenshot 2024-04-04 at 05 04 48

Raw text in a custom component

Screenshot 2024-04-04 at 05 04 56

Raw text in a prop

Screenshot 2024-04-04 at 14 50 45

'bsky-internal/avoid-unwrapped-text': [
'error',
{
impliedTextComponents: [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal is to get this list close to zero, maybe except H1..H6. For components that always return Text (e.g. links), I propose adding Text suffix and making the rule enforce that they actually return Text. Will need follow-up work.

Copy link

github-actions bot commented Apr 4, 2024

The Pull Request introduced fingerprint changes against the base commit: 8e393b1

Fingerprint diff
[
  {
    "type": "file",
    "filePath": "package.json",
    "reasons": [
      "expoConfigPlugins"
    ],
    "hash": "e63e4bc19f16de7405b22d617734da9061b47b13"
  }
]

Generated by PR labeler 🤖

@@ -234,6 +234,7 @@
"babel-preset-expo": "^10.0.0",
"detox": "^20.14.8",
"eslint": "^8.19.0",
"eslint-plugin-bsky-internal": "link:./eslint",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't forget to run yarn.

Copy link

github-actions bot commented Apr 4, 2024

Old size New size Diff
6.34 MB 6.34 MB 46 B (0.00%)

@gaearon gaearon force-pushed the add-eslint-msg-rule branch from 5b8747a to 2fc78e0 Compare April 4, 2024 13:44
@gaearon gaearon force-pushed the add-eslint-msg-rule branch from 2fc78e0 to 66f966c Compare April 4, 2024 13:49
@gaearon gaearon force-pushed the add-eslint-msg-rule branch from 8089e14 to fb9a516 Compare April 4, 2024 14:08
Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

Went through a bunch of test cases on this, works great. Really like the prop name idea as well.

@gaearon gaearon merged commit 4cc57f4 into main Apr 4, 2024
6 checks passed
@gaearon gaearon deleted the add-eslint-msg-rule branch April 4, 2024 16:32
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