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

Improve handling of invalid DimensionValue usage #36346

Closed
wants to merge 1 commit into from

Conversation

NickGerleman
Copy link
Contributor

Summary:

  1. Remove Paper native assertions for converting DimensionValue string to Yoga unit, and fix a case where Fabric could throw on invalid value.
  2. Move DimensionValue types in TypeScript to use template literal types, to show malformed strings in-editor, during typechecking. Update min TS version to allow this (in conformance with the min TS version used by DefinitelyTyped).

Changelog:
[General][Added] - Improve handling of invalid DimensionValue usage

Differential Revision: D43153075

@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. p: Facebook Partner: Facebook Partner fb-exported labels Mar 1, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43153075

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Mar 1, 2023
Summary:
Pull Request resolved: facebook#36346

1. Remove Paper native assertions for converting DimensionValue string to Yoga unit, and fix a case where Fabric could throw on invalid value.
2. Move DimensionValue types in TypeScript to use template literal types, to show malformed strings in-editor, during typechecking. Update min TS version to allow this (in conformance with the min TS version used by DefinitelyTyped).

Changelog:
[General][Added] - Improve handling of invalid DimensionValue usage

Differential Revision: D43153075

fbshipit-source-id: 84ca0e1fca88c97b44482e8e98d07c918a12bea1
@analysis-bot
Copy link

analysis-bot commented Mar 1, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,476,862 -233
android hermes armeabi-v7a 7,798,802 -277
android hermes x86 8,952,780 -309
android hermes x86_64 8,810,266 -321
android jsc arm64-v8a 9,107,940 -236
android jsc armeabi-v7a 8,304,572 -272
android jsc x86 9,158,738 -294
android jsc x86_64 9,417,990 -329

Base commit: 71157f6
Branch: main

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43153075

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

This pull request has been merged in 02e29ab.

rshest added a commit to rshest/react-native that referenced this pull request Mar 3, 2023
Summary:
## Changelog:

[General][Fixed] -

facebook#36346 added some typing improvements, however there was a typo in `AnimatableStringValue` type definition, that broke tests on CI.

Reviewed By: cortinico, cipolleschi, hoxyq

Differential Revision: D43770412

fbshipit-source-id: dc54a3593989050f5680c67c67fb61ed7d367c2f
facebook-github-bot pushed a commit that referenced this pull request Mar 3, 2023
Summary:
Pull Request resolved: #36366

## Changelog:

[General][Fixed] -

#36346 added some typing improvements, however there was a typo in `AnimatableStringValue` type definition, that broke tests on CI.

Reviewed By: cortinico, cipolleschi, hoxyq

Differential Revision: D43770412

fbshipit-source-id: 7b4f234b5cf04df9271e0c98cf51655c87e3bebb
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#36346

1. Remove Paper native assertions for converting DimensionValue string to Yoga unit, and fix a case where Fabric could throw on invalid value.
2. Move DimensionValue types in TypeScript to use template literal types, to show malformed strings in-editor, during typechecking. Update min TS version to allow this (in conformance with the min TS version used by DefinitelyTyped).

Changelog:
[General][Added] - Improve handling of invalid DimensionValue usage

Reviewed By: javache

Differential Revision: D43153075

fbshipit-source-id: db4e813df6e81cbd3158edad7c07c7a90c009803
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#36366

## Changelog:

[General][Fixed] -

facebook#36346 added some typing improvements, however there was a typo in `AnimatableStringValue` type definition, that broke tests on CI.

Reviewed By: cortinico, cipolleschi, hoxyq

Differential Revision: D43770412

fbshipit-source-id: 7b4f234b5cf04df9271e0c98cf51655c87e3bebb
@tom-sherman
Copy link

@NickGerleman doesn't DimensionValue also support px strings eg. "10px"?

@NickGerleman
Copy link
Contributor Author

@NickGerleman doesn't DimensionValue also support px strings eg. "10px"?

As of today, no. Please see:

  1. https://reactnative.dev/docs/height-and-width
  2. as a code example, where we explicitly maintain the invariant that strings are "auto" or "%"

There are some caveats. I think under the new architecture, the parser might end up being more permissive, and interpret anything starting with a number as a px value.

px value support, along with some more units, is likely coming in 0.74.

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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants