From 02e29abeada3d78dd7d90d1d89049cd1517afb55 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 2 Mar 2023 20:23:08 -0800 Subject: [PATCH] Improve handling of invalid DimensionValue usage (#36346) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- Libraries/StyleSheet/StyleSheetTypes.d.ts | 201 +++++++++--------- Libraries/StyleSheet/StyleSheetTypes.js | 48 +++-- React/Base/RCTConvert.m | 11 +- .../react/uimanager/LayoutShadowNode.java | 2 + .../renderer/components/view/conversions.h | 10 +- types/__typetests__/index.tsx | 32 +++ types/index.d.ts | 2 +- 7 files changed, 175 insertions(+), 131 deletions(-) diff --git a/Libraries/StyleSheet/StyleSheetTypes.d.ts b/Libraries/StyleSheet/StyleSheetTypes.d.ts index de3011daeb6159..37c6fea63d6c96 100644 --- a/Libraries/StyleSheet/StyleSheetTypes.d.ts +++ b/Libraries/StyleSheet/StyleSheetTypes.d.ts @@ -7,6 +7,7 @@ * @format */ +import {Animated} from '../Animated/Animated'; import {ImageResizeMode} from '../Image/ImageResizeMode'; import {ColorValue} from './StyleSheet'; @@ -17,6 +18,15 @@ type FlexAlignType = | 'stretch' | 'baseline'; +type DimensionValue = + | number + | 'auto' + | `${number}%` + | Animated.AnimatedNode + | null; +type AnimatableNumericValue = number | Animated.AnimatedNode; +type AnimatableStringValue = number | Animated.AnimatedNode; + /** * Flex Prop Types * @see https://reactnative.dev/docs/flexbox @@ -35,17 +45,17 @@ export interface FlexStyle { alignSelf?: 'auto' | FlexAlignType | undefined; aspectRatio?: number | string | undefined; borderBottomWidth?: number | undefined; - borderEndWidth?: number | string | undefined; + borderEndWidth?: number | undefined; borderLeftWidth?: number | undefined; borderRightWidth?: number | undefined; - borderStartWidth?: number | string | undefined; + borderStartWidth?: number | undefined; borderTopWidth?: number | undefined; borderWidth?: number | undefined; - bottom?: number | string | undefined; + bottom?: DimensionValue | undefined; display?: 'none' | 'flex' | undefined; - end?: number | string | undefined; + end?: DimensionValue | undefined; flex?: number | undefined; - flexBasis?: number | string | undefined; + flexBasis?: DimensionValue | undefined; flexDirection?: | 'row' | 'column' @@ -58,14 +68,14 @@ export interface FlexStyle { flexGrow?: number | undefined; flexShrink?: number | undefined; flexWrap?: 'wrap' | 'nowrap' | 'wrap-reverse' | undefined; - height?: number | string | undefined; - inset?: number | string | undefined; - insetBlock?: number | string | undefined; - insetBlockEnd?: number | string | undefined; - insetBlockStart?: number | string | undefined; - insetInline?: number | string | undefined; - insetInlineEnd?: number | string | undefined; - insetInlineStart?: number | string | undefined; + height?: DimensionValue | undefined; + inset?: DimensionValue | undefined; + insetBlock?: DimensionValue | undefined; + insetBlockEnd?: DimensionValue | undefined; + insetBlockStart?: DimensionValue | undefined; + insetInline?: DimensionValue | undefined; + insetInlineEnd?: DimensionValue | undefined; + insetInlineStart?: DimensionValue | undefined; justifyContent?: | 'flex-start' | 'flex-end' @@ -74,47 +84,47 @@ export interface FlexStyle { | 'space-around' | 'space-evenly' | undefined; - left?: number | string | undefined; - margin?: number | string | undefined; - marginBlock?: number | string | undefined; - marginBlockEnd?: number | string | undefined; - marginBlockStart?: number | string | undefined; - marginBottom?: number | string | undefined; - marginEnd?: number | string | undefined; - marginHorizontal?: number | string | undefined; - marginInline?: number | string | undefined; - marginInlineEnd?: number | string | undefined; - marginInlineStart?: number | string | undefined; - marginLeft?: number | string | undefined; - marginRight?: number | string | undefined; - marginStart?: number | string | undefined; - marginTop?: number | string | undefined; - marginVertical?: number | string | undefined; - maxHeight?: number | string | undefined; - maxWidth?: number | string | undefined; - minHeight?: number | string | undefined; - minWidth?: number | string | undefined; + left?: DimensionValue | undefined; + margin?: DimensionValue | undefined; + marginBlock?: DimensionValue | undefined; + marginBlockEnd?: DimensionValue | undefined; + marginBlockStart?: DimensionValue | undefined; + marginBottom?: DimensionValue | undefined; + marginEnd?: DimensionValue | undefined; + marginHorizontal?: DimensionValue | undefined; + marginInline?: DimensionValue | undefined; + marginInlineEnd?: DimensionValue | undefined; + marginInlineStart?: DimensionValue | undefined; + marginLeft?: DimensionValue | undefined; + marginRight?: DimensionValue | undefined; + marginStart?: DimensionValue | undefined; + marginTop?: DimensionValue | undefined; + marginVertical?: DimensionValue | undefined; + maxHeight?: DimensionValue | undefined; + maxWidth?: DimensionValue | undefined; + minHeight?: DimensionValue | undefined; + minWidth?: DimensionValue | undefined; overflow?: 'visible' | 'hidden' | 'scroll' | undefined; - padding?: number | string | undefined; - paddingBottom?: number | string | undefined; - paddingBlock?: number | string | undefined; - paddingBlockEnd?: number | string | undefined; - paddingBlockStart?: number | string | undefined; - paddingEnd?: number | string | undefined; - paddingHorizontal?: number | string | undefined; - paddingInline?: number | string | undefined; - paddingInlineEnd?: number | string | undefined; - paddingInlineStart?: number | string | undefined; - paddingLeft?: number | string | undefined; - paddingRight?: number | string | undefined; - paddingStart?: number | string | undefined; - paddingTop?: number | string | undefined; - paddingVertical?: number | string | undefined; + padding?: DimensionValue | undefined; + paddingBottom?: DimensionValue | undefined; + paddingBlock?: DimensionValue | undefined; + paddingBlockEnd?: DimensionValue | undefined; + paddingBlockStart?: DimensionValue | undefined; + paddingEnd?: DimensionValue | undefined; + paddingHorizontal?: DimensionValue | undefined; + paddingInline?: DimensionValue | undefined; + paddingInlineEnd?: DimensionValue | undefined; + paddingInlineStart?: DimensionValue | undefined; + paddingLeft?: DimensionValue | undefined; + paddingRight?: DimensionValue | undefined; + paddingStart?: DimensionValue | undefined; + paddingTop?: DimensionValue | undefined; + paddingVertical?: DimensionValue | undefined; position?: 'absolute' | 'relative' | undefined; - right?: number | string | undefined; - start?: number | string | undefined; - top?: number | string | undefined; - width?: number | string | undefined; + right?: DimensionValue | undefined; + start?: DimensionValue | undefined; + top?: DimensionValue | undefined; + width?: DimensionValue | undefined; zIndex?: number | undefined; /** @@ -125,61 +135,61 @@ export interface FlexStyle { export interface ShadowStyleIOS { shadowColor?: ColorValue | undefined; - shadowOffset?: {width: number; height: number} | undefined; - shadowOpacity?: number | undefined; + shadowOffset?: Readonly<{width: number; height: number}> | undefined; + shadowOpacity?: AnimatableNumericValue | undefined; shadowRadius?: number | undefined; } interface PerpectiveTransform { - perspective: number; + perspective: AnimatableNumericValue; } interface RotateTransform { - rotate: string; + rotate: AnimatableStringValue; } interface RotateXTransform { - rotateX: string; + rotateX: AnimatableStringValue; } interface RotateYTransform { - rotateY: string; + rotateY: AnimatableStringValue; } interface RotateZTransform { - rotateZ: string; + rotateZ: AnimatableStringValue; } interface ScaleTransform { - scale: number; + scale: AnimatableNumericValue; } interface ScaleXTransform { - scaleX: number; + scaleX: AnimatableNumericValue; } interface ScaleYTransform { - scaleY: number; + scaleY: AnimatableNumericValue; } interface TranslateXTransform { - translateX: number; + translateX: AnimatableNumericValue; } interface TranslateYTransform { - translateY: number; + translateY: AnimatableNumericValue; } interface SkewXTransform { - skewX: string; + skewX: AnimatableStringValue; } interface SkewYTransform { - skewY: string; + skewY: AnimatableStringValue; } interface MatrixTransform { - matrix: number[]; + matrix: AnimatableNumericValue[]; } export interface TransformsStyle { @@ -207,23 +217,23 @@ export interface TransformsStyle { /** * @deprecated Use rotate in transform prop instead. */ - rotation?: number | undefined; + rotation?: AnimatableNumericValue | undefined; /** * @deprecated Use scaleX in transform prop instead. */ - scaleX?: number | undefined; + scaleX?: AnimatableNumericValue | undefined; /** * @deprecated Use scaleY in transform prop instead. */ - scaleY?: number | undefined; + scaleY?: AnimatableNumericValue | undefined; /** * @deprecated Use translateX in transform prop instead. */ - translateX?: number | undefined; + translateX?: AnimatableNumericValue | undefined; /** * @deprecated Use translateY in transform prop instead. */ - translateY?: number | undefined; + translateY?: AnimatableNumericValue | undefined; } /** @@ -236,11 +246,10 @@ export interface ViewStyle extends FlexStyle, ShadowStyleIOS, TransformsStyle { borderBlockEndColor?: ColorValue | undefined; borderBlockStartColor?: ColorValue | undefined; borderBottomColor?: ColorValue | undefined; - borderBottomEndRadius?: number | undefined; - borderBottomLeftRadius?: number | undefined; - borderBottomRightRadius?: number | undefined; - borderBottomStartRadius?: number | undefined; - borderBottomWidth?: number | undefined; + borderBottomEndRadius?: AnimatableNumericValue | undefined; + borderBottomLeftRadius?: AnimatableNumericValue | undefined; + borderBottomRightRadius?: AnimatableNumericValue | undefined; + borderBottomStartRadius?: AnimatableNumericValue | undefined; borderColor?: ColorValue | undefined; /** * On iOS 13+, it is possible to change the corner curve of borders. @@ -248,25 +257,21 @@ export interface ViewStyle extends FlexStyle, ShadowStyleIOS, TransformsStyle { */ borderCurve?: 'circular' | 'continuous' | undefined; borderEndColor?: ColorValue | undefined; - borderEndEndRadius?: number | undefined; - borderEndStartRadius?: number | undefined; + borderEndEndRadius?: AnimatableNumericValue | undefined; + borderEndStartRadius?: AnimatableNumericValue | undefined; borderLeftColor?: ColorValue | undefined; - borderLeftWidth?: number | undefined; - borderRadius?: number | undefined; + borderRadius?: AnimatableNumericValue | undefined; borderRightColor?: ColorValue | undefined; - borderRightWidth?: number | undefined; borderStartColor?: ColorValue | undefined; - borderStartEndRadius?: number | undefined; - borderStartStartRadius?: number | undefined; + borderStartEndRadius?: AnimatableNumericValue | undefined; + borderStartStartRadius?: AnimatableNumericValue | undefined; borderStyle?: 'solid' | 'dotted' | 'dashed' | undefined; borderTopColor?: ColorValue | undefined; - borderTopEndRadius?: number | undefined; - borderTopLeftRadius?: number | undefined; - borderTopRightRadius?: number | undefined; - borderTopStartRadius?: number | undefined; - borderTopWidth?: number | undefined; - borderWidth?: number | undefined; - opacity?: number | undefined; + borderTopEndRadius?: AnimatableNumericValue | undefined; + borderTopLeftRadius?: AnimatableNumericValue | undefined; + borderTopRightRadius?: AnimatableNumericValue | undefined; + borderTopStartRadius?: AnimatableNumericValue | undefined; + opacity?: AnimatableNumericValue | undefined; /** * Sets the elevation of a view, using Android's underlying * [elevation API](https://developer.android.com/training/material/shadows-clipping.html#Elevation). @@ -290,7 +295,6 @@ export type FontVariant = | 'proportional-nums'; export interface TextStyleIOS extends ViewStyle { fontVariant?: FontVariant[] | undefined; - letterSpacing?: number | undefined; textDecorationColor?: ColorValue | undefined; textDecorationStyle?: 'solid' | 'double' | 'dotted' | 'dashed' | undefined; writingDirection?: 'auto' | 'ltr' | 'rtl' | undefined; @@ -351,17 +355,16 @@ export interface TextStyle extends TextStyleIOS, TextStyleAndroid, ViewStyle { export interface ImageStyle extends FlexStyle, ShadowStyleIOS, TransformsStyle { resizeMode?: ImageResizeMode | undefined; backfaceVisibility?: 'visible' | 'hidden' | undefined; - borderBottomLeftRadius?: number | undefined; - borderBottomRightRadius?: number | undefined; + borderBottomLeftRadius?: AnimatableNumericValue | undefined; + borderBottomRightRadius?: AnimatableNumericValue | undefined; backgroundColor?: ColorValue | undefined; borderColor?: ColorValue | undefined; - borderWidth?: number | undefined; - borderRadius?: number | undefined; - borderTopLeftRadius?: number | undefined; - borderTopRightRadius?: number | undefined; + borderRadius?: AnimatableNumericValue | undefined; + borderTopLeftRadius?: AnimatableNumericValue | undefined; + borderTopRightRadius?: AnimatableNumericValue | undefined; overflow?: 'visible' | 'hidden' | undefined; overlayColor?: ColorValue | undefined; tintColor?: ColorValue | undefined; - opacity?: number | undefined; + opacity?: AnimatableNumericValue | undefined; objectFit?: 'cover' | 'contain' | 'fill' | 'scale-down' | undefined; } diff --git a/Libraries/StyleSheet/StyleSheetTypes.js b/Libraries/StyleSheet/StyleSheetTypes.js index 4fc70eed012233..5d9b56c3f4ea6c 100644 --- a/Libraries/StyleSheet/StyleSheetTypes.js +++ b/Libraries/StyleSheet/StyleSheetTypes.js @@ -33,7 +33,9 @@ export type EdgeInsetsValue = { right: number, bottom: number, }; -export type DimensionValue = null | number | string | AnimatedNode; + +export type DimensionValue = number | string | 'auto' | AnimatedNode | null; +export type AnimatableNumericValue = number | AnimatedNode; /** * React Native's layout system is based on Flexbox and is powered both @@ -671,7 +673,7 @@ export type ____ShadowStyle_InternalCore = $ReadOnly<{ * Sets the drop shadow opacity (multiplied by the color's alpha component) * @platform ios */ - shadowOpacity?: number | AnimatedNode, + shadowOpacity?: AnimatableNumericValue, /** * Sets the drop shadow blur radius * @platform ios @@ -701,28 +703,28 @@ export type ____ViewStyle_InternalCore = $ReadOnly<{ borderBlockColor?: ____ColorValue_Internal, borderBlockEndColor?: ____ColorValue_Internal, borderBlockStartColor?: ____ColorValue_Internal, - borderRadius?: number | AnimatedNode, - borderBottomEndRadius?: number | AnimatedNode, - borderBottomLeftRadius?: number | AnimatedNode, - borderBottomRightRadius?: number | AnimatedNode, - borderBottomStartRadius?: number | AnimatedNode, - borderEndEndRadius?: number | AnimatedNode, - borderEndStartRadius?: number | AnimatedNode, - borderStartEndRadius?: number | AnimatedNode, - borderStartStartRadius?: number | AnimatedNode, - borderTopEndRadius?: number | AnimatedNode, - borderTopLeftRadius?: number | AnimatedNode, - borderTopRightRadius?: number | AnimatedNode, - borderTopStartRadius?: number | AnimatedNode, + borderRadius?: AnimatableNumericValue, + borderBottomEndRadius?: AnimatableNumericValue, + borderBottomLeftRadius?: AnimatableNumericValue, + borderBottomRightRadius?: AnimatableNumericValue, + borderBottomStartRadius?: AnimatableNumericValue, + borderEndEndRadius?: AnimatableNumericValue, + borderEndStartRadius?: AnimatableNumericValue, + borderStartEndRadius?: AnimatableNumericValue, + borderStartStartRadius?: AnimatableNumericValue, + borderTopEndRadius?: AnimatableNumericValue, + borderTopLeftRadius?: AnimatableNumericValue, + borderTopRightRadius?: AnimatableNumericValue, + borderTopStartRadius?: AnimatableNumericValue, borderStyle?: 'solid' | 'dotted' | 'dashed', - borderWidth?: number | AnimatedNode, - borderBottomWidth?: number | AnimatedNode, - borderEndWidth?: number | AnimatedNode, - borderLeftWidth?: number | AnimatedNode, - borderRightWidth?: number | AnimatedNode, - borderStartWidth?: number | AnimatedNode, - borderTopWidth?: number | AnimatedNode, - opacity?: number | AnimatedNode, + borderWidth?: AnimatableNumericValue, + borderBottomWidth?: AnimatableNumericValue, + borderEndWidth?: AnimatableNumericValue, + borderLeftWidth?: AnimatableNumericValue, + borderRightWidth?: AnimatableNumericValue, + borderStartWidth?: AnimatableNumericValue, + borderTopWidth?: AnimatableNumericValue, + opacity?: AnimatableNumericValue, elevation?: number, pointerEvents?: 'auto' | 'none' | 'box-none' | 'box-only', }>; diff --git a/React/Base/RCTConvert.m b/React/Base/RCTConvert.m index a2dfa2bcf6dd89..f24c09b539ae73 100644 --- a/React/Base/RCTConvert.m +++ b/React/Base/RCTConvert.m @@ -992,12 +992,15 @@ + (YGValue)YGValue:(id)json if ([s isEqualToString:@"auto"]) { return (YGValue){YGUndefined, YGUnitAuto}; } else if ([s hasSuffix:@"%"]) { - return (YGValue){[[s substringToIndex:s.length] floatValue], YGUnitPercent}; + float floatValue; + if ([[NSScanner scannerWithString:s] scanFloat:&floatValue]) { + return (YGValue){floatValue, YGUnitPercent}; + } } else { - RCTLogConvertError(json, @"a YGValue. Did you forget the % or pt suffix?"); + RCTLogAdvice( + @"\"%@\" is not a valid dimension. Dimensions must be a number, \"auto\", or a string suffixed with \"%%\".", + s); } - } else { - RCTLogConvertError(json, @"a YGValue."); } return YGValueUndefined; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java index 07cd486b5477f4..46b833a053a2dc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java @@ -62,6 +62,8 @@ void setFromDynamic(Dynamic dynamic) { value = Float.parseFloat(s.substring(0, s.length() - 1)); } else { FLog.w(ReactConstants.TAG, "Unknown value: " + s); + unit = YogaUnit.UNDEFINED; + value = YogaConstants.UNDEFINED; } } else if (dynamic.getType() == ReadableType.Number) { unit = YogaUnit.POINT; diff --git a/ReactCommon/react/renderer/components/view/conversions.h b/ReactCommon/react/renderer/components/view/conversions.h index b78d264d8d439a..ad087df7877765 100644 --- a/ReactCommon/react/renderer/components/view/conversions.h +++ b/ReactCommon/react/renderer/components/view/conversions.h @@ -425,10 +425,12 @@ inline void fromRawValue( return; } else { if (stringValue.back() == '%') { - result = YGValue{ - folly::to(stringValue.substr(0, stringValue.length() - 1)), - YGUnitPercent}; - return; + auto tryValue = folly::tryTo( + std::string_view(stringValue).substr(0, stringValue.length() - 1)); + if (tryValue.hasValue()) { + result = YGValue{tryValue.value(), YGUnitPercent}; + return; + } } else { auto tryValue = folly::tryTo(stringValue); if (tryValue.hasValue()) { diff --git a/types/__typetests__/index.tsx b/types/__typetests__/index.tsx index 848a7b5239dfd2..23b15ad3171d6e 100644 --- a/types/__typetests__/index.tsx +++ b/types/__typetests__/index.tsx @@ -26,6 +26,7 @@ import { AccessibilityInfo, ActionSheetIOS, Alert, + Animated, AppState, AppStateStatus, Appearance, @@ -247,6 +248,37 @@ const s = StyleSheet.create({ }); const f1: TextStyle = s.shouldWork; +const styleDimensionValueValidPoint: ViewStyle = { + width: 1, +}; + +const styleDimensionValueValidAuto: ViewStyle = { + width: 'auto', +}; + +const styleDimensionValueValidPct: ViewStyle = { + width: '5%', +}; + +const styleDimensionValueValidAnimated: ViewStyle = { + width: new Animated.Value(5), +}; + +const styleDimensionValueInvalid1: ViewStyle = { + // @ts-expect-error + width: '5', +}; + +const styleDimensionValueInvalid2: ViewStyle = { + // @ts-expect-error + width: '5px', +}; + +const styleDimensionValueInvalid3: ViewStyle = { + // @ts-expect-error + width: 'A%', +}; + // StyleSheet.compose // It creates a new style object by composing two existing styles const composeTextStyle: StyleProp = { diff --git a/types/index.d.ts b/types/index.d.ts index 268383e915a0fb..d50dd04b12b602 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -51,7 +51,7 @@ // Mateusz Wit // Saad Najmi // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped -// TypeScript Version: 3.0 +// Minimum TypeScript Version: 4.1 /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// //