Skip to content

Commit 25c673e

Browse files
elicwhitefacebook-github-bot
authored andcommitted
Fixing schema types for component command params of Arrays (#48476)
Summary: Pull Request resolved: #48476 Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript. We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules. # More Information: Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands. ## The problem: The [CodegenSchema](https://www.internalfb.com/code/fbsource/[d3ab2f79b377]/xplat/js/react-native-github/packages/react-native-codegen/src/CodegenSchema.js?lines=220) should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema. ## The details: Support for Arrays as arguments in commands was added to the Codegen in D51866557. [Code Pointer](https://fburl.com/code/8yy1rm0p) The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types. ``` interface NativeCommands { +addOverlays: ( viewRef: React.ElementRef<NativeType>, overlayColorsReadOnly: $ReadOnlyArray<string>, ) } ``` This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only ``` { readonly type: 'ArrayTypeAnnotation'; readonly elementType: | Int32TypeAnnotation | DoubleTypeAnnotation | FloatTypeAnnotation | BooleanTypeAnnotation | StringTypeAnnotation }; ``` However, because the Parsers are treating the input type as `any`, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches. The Flow parser just passes it through: ``` { type: 'ArrayTypeAnnotation', elementType: { // TODO: T172453752 support complex type annotation for array element type: paramValue.typeParameters.params[0].type, } ``` Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits. ``` function getPrimitiveTypeAnnotation(type: string): $FlowFixMe { switch (type) { case 'Int32': return { type: 'Int32TypeAnnotation', }; case 'Double': return { type: 'DoubleTypeAnnotation', }; case 'Float': return { type: 'FloatTypeAnnotation', }; case 'TSBooleanKeyword': return { type: 'BooleanTypeAnnotation', }; case 'Stringish': case 'TSStringKeyword': return { type: 'StringTypeAnnotation', }; default: throw new Error(`Unknown primitive type "${type}"`); } } ``` [DebuggingOverlay](https://fburl.com/code/zfe3ipq7) is abusing this gap in the Flow parser by sticking an Array of Objects in. ``` export type ElementRectangle = { x: number, y: number, width: number, height: number, }; ... +highlightElements: ( viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>, elements: $ReadOnlyArray<ElementRectangle>, ) => void; ... ``` This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators. The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema. ``` { "name": "highlightElements", "optional": false, "typeAnnotation": { "type": "FunctionTypeAnnotation", "params": [ { "name": "elements", "optional": false, "typeAnnotation": { "type": "ArrayTypeAnnotation", "elementType": { "type": "GenericTypeAnnotation" } } } ], "returnTypeAnnotation": { "type": "VoidTypeAnnotation" } }, ``` The TypeScript parser fails with `Error: Unsupported type annotation: GenericTypeAnnotation`. The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays. ``` // ios elements:(const NSArray *)elements // android ReadableArray locations ``` ## So how do I fix this? I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema. 1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place) 2. **(This is the approach taken in this diff)** We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side. 3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add `aliasMap` and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code. ``` export type ObjectAlias = {| x: number, y: number, |}; export interface Spec extends TurboModule { +getAlias: (a: ObjectAlias) => string; } ``` stores the ObjectAlias in the schema ``` { "aliasMap": { "ObjectAlias": { "type": "ObjectTypeAnnotation", "properties": [ { "name": "x", "optional": false, "typeAnnotation": { "type": "NumberTypeAnnotation" } }, { "name": "y", "optional": false, "typeAnnotation": { "type": "NumberTypeAnnotation" } }, ] } }, "spec": { "methods": [ { "name": "getAlias", "optional": false, "typeAnnotation": { "type": "FunctionTypeAnnotation", "returnTypeAnnotation": { "type": "StringTypeAnnotation" }, "params": [ { "name": "a", "optional": false, "typeAnnotation": { "type": "TypeAliasTypeAnnotation", "name": "ObjectAlias" } } ] } } ] } } ``` and then generates the appropriate structs on the native side and generates [this](https://www.internalfb.com/code/fbsource/[d3ab2f79b377]/xplat/js/react-native-github/packages/react-native-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleHObjCpp-test.js.snap?lines=818) ``` Reviewed By: hoxyq Differential Revision: D67806838 fbshipit-source-id: 31f20455c816fdb6b1a86f8f9d0f6f7d0a452754
1 parent c748b44 commit 25c673e

File tree

13 files changed

+319
-27
lines changed

13 files changed

+319
-27
lines changed

packages/react-native-codegen/src/CodegenSchema.d.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,11 @@ export type ComponentCommandArrayTypeAnnotation = ArrayTypeAnnotation<
148148
| DoubleTypeAnnotation
149149
| FloatTypeAnnotation
150150
| Int32TypeAnnotation
151+
// Mixed is not great. This generally means its a type alias to another type
152+
// like an object or union. Ideally we'd encode that type in the schema so the compat-check can
153+
// validate those deeper objects for breaking changes and the generators can do something smarter.
154+
// As of now, the generators just create ReadableMap or (const NSArray *) which are untyped
155+
| MixedTypeAnnotation
151156
>;
152157

153158
export interface ArrayTypeAnnotation<T> {

packages/react-native-codegen/src/CodegenSchema.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,12 @@ export type ComponentCommandArrayTypeAnnotation = ArrayTypeAnnotation<
163163
| StringTypeAnnotation
164164
| DoubleTypeAnnotation
165165
| FloatTypeAnnotation
166-
| Int32TypeAnnotation,
166+
| Int32TypeAnnotation
167+
// Mixed is not great. This generally means its a type alias to another type
168+
// like an object or union. Ideally we'd encode that type in the schema so the compat-check can
169+
// validate those deeper objects for breaking changes and the generators can do something smarter.
170+
// As of now, the generators just create ReadableMap or (const NSArray *) which are untyped
171+
| MixedTypeAnnotation,
167172
>;
168173

169174
export type ArrayTypeAnnotation<+T> = $ReadOnly<{

packages/react-native-codegen/src/generators/components/__test_fixtures__/fixtures.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1693,6 +1693,16 @@ const COMMANDS: SchemaType = {
16931693
type: 'BooleanTypeAnnotation',
16941694
},
16951695
},
1696+
{
1697+
name: 'locations',
1698+
optional: false,
1699+
typeAnnotation: {
1700+
type: 'ArrayTypeAnnotation',
1701+
elementType: {
1702+
type: 'MixedTypeAnnotation',
1703+
},
1704+
},
1705+
},
16961706
],
16971707
returnTypeAnnotation: {
16981708
type: 'VoidTypeAnnotation',

packages/react-native-codegen/src/generators/components/__tests__/__snapshots__/GenerateComponentHObjCpp-test.js.snap

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ NS_ASSUME_NONNULL_BEGIN
119119
120120
@protocol RCTCommandNativeComponentViewProtocol <NSObject>
121121
- (void)flashScrollIndicators;
122-
- (void)allTypes:(NSInteger)x y:(float)y z:(double)z message:(NSString *)message animated:(BOOL)animated;
122+
- (void)allTypes:(NSInteger)x y:(float)y z:(double)z message:(NSString *)message animated:(BOOL)animated locations:(const NSArray *)locations;
123123
@end
124124
125125
RCT_EXTERN inline void RCTCommandNativeComponentHandleCommand(
@@ -143,8 +143,8 @@ RCT_EXTERN inline void RCTCommandNativeComponentHandleCommand(
143143
144144
if ([commandName isEqualToString:@\\"allTypes\\"]) {
145145
#if RCT_DEBUG
146-
if ([args count] != 5) {
147-
RCTLogError(@\\"%@ command %@ received %d arguments, expected %d.\\", @\\"CommandNativeComponent\\", commandName, (int)[args count], 5);
146+
if ([args count] != 6) {
147+
RCTLogError(@\\"%@ command %@ received %d arguments, expected %d.\\", @\\"CommandNativeComponent\\", commandName, (int)[args count], 6);
148148
return;
149149
}
150150
#endif
@@ -189,7 +189,15 @@ NSObject *arg4 = args[4];
189189
#endif
190190
BOOL animated = [(NSNumber *)arg4 boolValue];
191191
192-
[componentView allTypes:x y:y z:z message:message animated:animated];
192+
NSObject *arg5 = args[5];
193+
#if RCT_DEBUG
194+
if (!RCTValidateTypeOfViewCommandArgument(arg5, [NSArray class], @\\"array\\", @\\"CommandNativeComponent\\", commandName, @\\"6th\\")) {
195+
return;
196+
}
197+
#endif
198+
const NSArray * locations = (NSArray *)arg5;
199+
200+
[componentView allTypes:x y:y z:z message:message animated:animated locations:locations];
193201
return;
194202
}
195203

packages/react-native-codegen/src/generators/components/__tests__/__snapshots__/GeneratePropsJavaDelegate-test.js.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ public class CommandNativeComponentManagerDelegate<T extends View, U extends Bas
224224
mViewManager.flashScrollIndicators(view);
225225
break;
226226
case \\"allTypes\\":
227-
mViewManager.allTypes(view, args.getInt(0), (float) args.getDouble(1), args.getDouble(2), args.getString(3), args.getBoolean(4));
227+
mViewManager.allTypes(view, args.getInt(0), (float) args.getDouble(1), args.getDouble(2), args.getString(3), args.getBoolean(4), args.getArray(5));
228228
break;
229229
}
230230
}

packages/react-native-codegen/src/generators/components/__tests__/__snapshots__/GeneratePropsJavaInterface-test.js.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,12 @@ Map {
118118
package com.facebook.react.viewmanagers;
119119
120120
import android.view.View;
121+
import com.facebook.react.bridge.ReadableArray;
121122
122123
public interface CommandNativeComponentManagerInterface<T extends View> {
123124
// No props
124125
void flashScrollIndicators(T view);
125-
void allTypes(T view, int x, float y, double z, String message, boolean animated);
126+
void allTypes(T view, int x, float y, double z, String message, boolean animated, ReadableArray locations);
126127
}
127128
",
128129
}

packages/react-native-codegen/src/generators/components/__tests__/__snapshots__/GenerateViewConfigJs-test.js.snap

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,8 @@ export const Commands = {
187187
dispatchCommand(ref, \\"flashScrollIndicators\\", []);
188188
},
189189
190-
allTypes(ref, x, y, z, message, animated) {
191-
dispatchCommand(ref, \\"allTypes\\", [x, y, z, message, animated]);
190+
allTypes(ref, x, y, z, message, animated, locations) {
191+
dispatchCommand(ref, \\"allTypes\\", [x, y, z, message, animated, locations]);
192192
}
193193
};
194194
",

packages/react-native-codegen/src/parsers/flow/components/__test_fixtures__/fixtures.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -951,10 +951,18 @@ interface NativeCommands {
951951
z: Double,
952952
animated: boolean,
953953
): void;
954+
+arrayArgs: (
955+
viewRef: React.ElementRef<NativeType>,
956+
booleanArray: $ReadOnlyArray<boolean>,
957+
stringArray: $ReadOnlyArray<string>,
958+
floatArray: $ReadOnlyArray<Float>,
959+
intArray: $ReadOnlyArray<Int32>,
960+
doubleArray: $ReadOnlyArray<Double>,
961+
) => void;
954962
}
955963
956964
export const Commands = codegenNativeCommands<NativeCommands>({
957-
supportedCommands: ['handleRootTag', 'hotspotUpdate', 'scrollTo'],
965+
supportedCommands: ['handleRootTag', 'hotspotUpdate', 'scrollTo', 'arrayArgs'],
958966
});
959967
960968
export default (codegenNativeComponent<ModuleProps>(
@@ -985,6 +993,10 @@ import type {HostComponent} from 'react-native';
985993
export type Boolean = boolean;
986994
export type Int = Int32;
987995
export type Void = void;
996+
export type Locations = {
997+
x: number,
998+
y: number,
999+
}
9881000
9891001
export type ModuleProps = $ReadOnly<{|
9901002
...ViewProps,
@@ -1006,6 +1018,7 @@ interface NativeCommands {
10061018
overlayColorsReadOnly: $ReadOnlyArray<string>,
10071019
overlayColorsArray: Array<string>,
10081020
overlayColorsArrayAnnotation: string[],
1021+
overlayLocations: $ReadOnlyArray<Locations>,
10091022
) => void;
10101023
}
10111024

packages/react-native-codegen/src/parsers/flow/components/__tests__/__snapshots__/component-parser-test.js.snap

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1447,6 +1447,68 @@ exports[`RN Codegen Flow Parser can generate fixture COMMANDS_DEFINED_WITH_ALL_T
14471447
'type': 'VoidTypeAnnotation'
14481448
}
14491449
}
1450+
},
1451+
{
1452+
'name': 'arrayArgs',
1453+
'optional': false,
1454+
'typeAnnotation': {
1455+
'type': 'FunctionTypeAnnotation',
1456+
'params': [
1457+
{
1458+
'name': 'booleanArray',
1459+
'optional': false,
1460+
'typeAnnotation': {
1461+
'type': 'ArrayTypeAnnotation',
1462+
'elementType': {
1463+
'type': 'BooleanTypeAnnotation'
1464+
}
1465+
}
1466+
},
1467+
{
1468+
'name': 'stringArray',
1469+
'optional': false,
1470+
'typeAnnotation': {
1471+
'type': 'ArrayTypeAnnotation',
1472+
'elementType': {
1473+
'type': 'StringTypeAnnotation'
1474+
}
1475+
}
1476+
},
1477+
{
1478+
'name': 'floatArray',
1479+
'optional': false,
1480+
'typeAnnotation': {
1481+
'type': 'ArrayTypeAnnotation',
1482+
'elementType': {
1483+
'type': 'FloatTypeAnnotation'
1484+
}
1485+
}
1486+
},
1487+
{
1488+
'name': 'intArray',
1489+
'optional': false,
1490+
'typeAnnotation': {
1491+
'type': 'ArrayTypeAnnotation',
1492+
'elementType': {
1493+
'type': 'Int32TypeAnnotation'
1494+
}
1495+
}
1496+
},
1497+
{
1498+
'name': 'doubleArray',
1499+
'optional': false,
1500+
'typeAnnotation': {
1501+
'type': 'ArrayTypeAnnotation',
1502+
'elementType': {
1503+
'type': 'DoubleTypeAnnotation'
1504+
}
1505+
}
1506+
}
1507+
],
1508+
'returnTypeAnnotation': {
1509+
'type': 'VoidTypeAnnotation'
1510+
}
1511+
}
14501512
}
14511513
]
14521514
}
@@ -4826,6 +4888,16 @@ exports[`RN Codegen Flow Parser can generate fixture COMMANDS_WITH_EXTERNAL_TYPE
48264888
'type': 'StringTypeAnnotation'
48274889
}
48284890
}
4891+
},
4892+
{
4893+
'name': 'overlayLocations',
4894+
'optional': false,
4895+
'typeAnnotation': {
4896+
'type': 'ArrayTypeAnnotation',
4897+
'elementType': {
4898+
'type': 'MixedTypeAnnotation'
4899+
}
4900+
}
48294901
}
48304902
],
48314903
'returnTypeAnnotation': {

packages/react-native-codegen/src/parsers/flow/components/commands.js

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import type {
1414
CommandParamTypeAnnotation,
1515
CommandTypeAnnotation,
16+
ComponentCommandArrayTypeAnnotation,
1617
NamedShape,
1718
} from '../../../CodegenSchema.js';
1819
import type {TypeDeclarationMap} from '../../utils';
@@ -109,19 +110,15 @@ function buildCommandSchema(
109110
}
110111
returnType = {
111112
type: 'ArrayTypeAnnotation',
112-
elementType: {
113-
// TODO: T172453752 support complex type annotation for array element
114-
type: paramValue.typeParameters.params[0].type,
115-
},
113+
elementType: getCommandArrayElementTypeType(
114+
paramValue.typeParameters.params[0],
115+
),
116116
};
117117
break;
118118
case 'ArrayTypeAnnotation':
119119
returnType = {
120120
type: 'ArrayTypeAnnotation',
121-
elementType: {
122-
// TODO: T172453752 support complex type annotation for array element
123-
type: paramValue.elementType.type,
124-
},
121+
elementType: getCommandArrayElementTypeType(paramValue.elementType),
125122
};
126123
break;
127124
default:
@@ -151,6 +148,66 @@ function buildCommandSchema(
151148
};
152149
}
153150

151+
type Allowed = ComponentCommandArrayTypeAnnotation['elementType'];
152+
153+
function getCommandArrayElementTypeType(inputType: mixed): Allowed {
154+
// TODO: T172453752 support more complex type annotation for array element
155+
if (typeof inputType !== 'object') {
156+
throw new Error('Expected an object');
157+
}
158+
159+
const type = inputType?.type;
160+
161+
if (inputType == null || typeof type !== 'string') {
162+
throw new Error('Command array element type must be a string');
163+
}
164+
165+
switch (type) {
166+
case 'BooleanTypeAnnotation':
167+
return {
168+
type: 'BooleanTypeAnnotation',
169+
};
170+
case 'StringTypeAnnotation':
171+
return {
172+
type: 'StringTypeAnnotation',
173+
};
174+
case 'GenericTypeAnnotation':
175+
const name = typeof inputType.id === 'object' ? inputType.id?.name : null;
176+
177+
if (typeof name !== 'string') {
178+
throw new Error(
179+
'Expected GenericTypeAnnotation AST name to be a string',
180+
);
181+
}
182+
183+
switch (name) {
184+
case 'Int32':
185+
return {
186+
type: 'Int32TypeAnnotation',
187+
};
188+
case 'Float':
189+
return {
190+
type: 'FloatTypeAnnotation',
191+
};
192+
case 'Double':
193+
return {
194+
type: 'DoubleTypeAnnotation',
195+
};
196+
default:
197+
// This is not a great solution. This generally means its a type alias to another type
198+
// like an object or union. Ideally we'd encode that in the schema so the compat-check can
199+
// validate those deeper objects for breaking changes and the generators can do something smarter.
200+
// As of now, the generators just create ReadableMap or (const NSArray *) which are untyped
201+
return {
202+
type: 'MixedTypeAnnotation',
203+
};
204+
}
205+
206+
default:
207+
throw new Error(`Unsupported array element type ${type}`);
208+
}
209+
}
210+
154211
function getCommands(
155212
commandTypeAST: $ReadOnlyArray<EventTypeAST>,
156213
types: TypeDeclarationMap,

0 commit comments

Comments
 (0)