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

chore: stricter TS check for transform style #38348

Closed

Conversation

vonovak
Copy link
Collaborator

@vonovak vonovak commented Jul 14, 2023

Summary:

This improves the strictness of TS typings for transform on View's style prop. Consider the following example, with what TS reports in case of errors, using RN 0.72.3. The ❌ / ✅ symbols indicate whether TS is happy with the code

 <View style={{ transform: [{ scale: undefined }] }} />              //  TS2769: No overload matches this call. 
 <View style={{ transform: [{ something: 1 }] }} />                  //  TS2769: No overload matches this call.
 <View style={{ transform: [{ scale: 1 }, { rotate: '90deg' }] }} />
 <View style={{ transform: [{ scale: 1, translateX: 1 }] }} /> // this is WRONG, corrected in the next row
 <View style={{ transform: [{ scale: 1 }, { translateX: 1 }] }} />

With this change, TS will report an error even for line 4

 <View style={{ transform: [{ scale: undefined }] }} />              //  TS2769: No overload matches this call. 
 <View style={{ transform: [{ something: 1 }] }} />                  //  TS2769: No overload matches this call.
 <View style={{ transform: [{ scale: 1 }, { rotate: '90deg' }] }} />
 <View style={{ transform: [{ scale: 1, translateX: 1 }] }} />      //  TS2769: No overload matches this call.
 <View style={{ transform: [{ scale: 1 }, { translateX: 1 }] }} />

Changelog:

Test Plan:

tested locally with the example given above; also added a TS type test

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 14, 2023
@analysis-bot
Copy link

analysis-bot commented Jul 14, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,834,261 -65
android hermes armeabi-v7a 8,143,547 -9
android hermes x86 9,338,645 +45
android hermes x86_64 9,181,669 +39
android jsc arm64-v8a 9,446,930 -1
android jsc armeabi-v7a 8,628,558 -1
android jsc x86 9,529,024 -1
android jsc x86_64 9,772,803 -1

Base commit: 06668fc
Branch: main

@vonovak vonovak force-pushed the chore/transform-stricker-types branch from 7898fd5 to 4952fa3 Compare July 14, 2023 13:49
@vonovak vonovak force-pushed the chore/transform-stricker-types branch from 4952fa3 to 50cc6bf Compare July 14, 2023 14:05
@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jul 18, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in e414713.

juniorklawa pushed a commit to juniorklawa/react-native that referenced this pull request Jul 20, 2023
Summary:
This improves the strictness of TS typings for `transform` on `View`'s `style` prop. Consider the following example, with what TS reports in case of errors, using RN 0.72.3. The ❌ / ✅ symbols indicate whether TS is happy with the code

```tsx
❌ <View style={{ transform: [{ scale: undefined }] }} />              //  TS2769: No overload matches this call.
❌ <View style={{ transform: [{ something: 1 }] }} />                  //  TS2769: No overload matches this call.
✅ <View style={{ transform: [{ scale: 1 }, { rotate: '90deg' }] }} />
✅ <View style={{ transform: [{ scale: 1, translateX: 1 }] }} /> // this is WRONG, corrected in the next row
✅ <View style={{ transform: [{ scale: 1 }, { translateX: 1 }] }} />
```

With this change, TS will report an error even for line 4

```tsx
❌ <View style={{ transform: [{ scale: undefined }] }} />              //  TS2769: No overload matches this call.
❌ <View style={{ transform: [{ something: 1 }] }} />                  //  TS2769: No overload matches this call.
✅ <View style={{ transform: [{ scale: 1 }, { rotate: '90deg' }] }} />
❌ <View style={{ transform: [{ scale: 1, translateX: 1 }] }} />      //  TS2769: No overload matches this call.
✅ <View style={{ transform: [{ scale: 1 }, { translateX: 1 }] }} />
```

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

[GENERAL] [CHANGED] - stricter TS check for transform style

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38348

Test Plan: tested locally with the example given above; also added a TS type test

Reviewed By: rshest

Differential Revision: D47526366

Pulled By: NickGerleman

fbshipit-source-id: 5bd007ce29509ccdfce74c3864dee24290d9a175
@NickGerleman
Copy link
Contributor

@vonovak from #41869 it looks like this change may cause typechecking issues when not using TS strict mode.

(After the holidays) we will probably want to look at fixing this, or potentially reverting the change, since RN typings should support users not on strict mode.

@NickGerleman
Copy link
Contributor

Hmm, I learned that DefinitelyTyped itself checks with strictNullChecks, and that our own type tests model that, and are clean with this change. https://github.com/facebook/react-native/blob/main/packages/react-native/types/tsconfig.json

I think it DT only checks against strictNullChecks, we should require it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants