Handle (T) and undefined properly in turbo module component codegen (#…

…34693) Summary: In `buildEventSchema` and `buildPropSchema`, they check into property types to see if the given property could be converted into an event schema or a property schema. The original implementation only handles limited cases, I refactor them and make them easier to maintain. In `getPropertyType` in `events.js`, it handles `(T)` at a wrong place, fixed. In `getPropertyType` in `props.js`, it doesn't handle `(T)`, fixed. And I also fixed some other issues to make the codegen reports error better. There are many duplicated test cases that cover every piece of the code, I changed some of them so that it tests both original cases and new cases. ## Changelog [General] [Changed] - Handle (T) and undefined properly in turbo module component codegen Pull Request resolved: #34693 Test Plan: `yarn jest` passed in `packages/react-native-codegen` Reviewed By: NickGerleman Differential Revision: D39647075 Pulled By: cipolleschi fbshipit-source-id: 8e1df2b54aab37b7151d0bf74260e2eba0602777
facebook · Sep 22, 2022 · 205cc9b · 205cc9b
1 parent 10e47b8
commit 205cc9b
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 102 deletions.
diff --git a/...ages/react-native-codegen/src/parsers/typescript/components/__test_fixtures__/fixtures.js b/...ages/react-native-codegen/src/parsers/typescript/components/__test_fixtures__/fixtures.js
@@ -19,9 +19,9 @@ const EVENT_DEFINITION = `
   boolean_optional_both?: boolean | null | undefined;
 
   string_required: string;
-  string_optional_key?: string;
-  string_optional_value: string | null | undefined;
-  string_optional_both?: string | null | undefined;
+  string_optional_key?: (string);
+  string_optional_value: (string) | null | undefined;
+  string_optional_both?: (string | null | undefined);
 
   double_required: Double;
   double_optional_key?: Double;
@@ -764,23 +764,23 @@ export interface ModuleProps extends ViewProps {
     }>
   >;
 
-  onDirectEventDefinedInlineOptionalKey?: DirectEventHandler<
+  onDirectEventDefinedInlineOptionalKey?: (DirectEventHandler<
     Readonly<{
       ${EVENT_DEFINITION}
     }>
-  >;
+  >);
 
-  onDirectEventDefinedInlineOptionalValue: DirectEventHandler<
+  onDirectEventDefinedInlineOptionalValue: (DirectEventHandler<
     Readonly<{
       ${EVENT_DEFINITION}
     }>
-  > | null | undefined;
+  >) | null | undefined;
 
-  onDirectEventDefinedInlineOptionalBoth?: DirectEventHandler<
+  onDirectEventDefinedInlineOptionalBoth?: (DirectEventHandler<
     Readonly<{
       ${EVENT_DEFINITION}
     }>
-  > | null | undefined;
+  > | null | undefined);
 
   onDirectEventDefinedInlineWithPaperName?: DirectEventHandler<
     Readonly<{
@@ -857,12 +857,12 @@ export interface ModuleProps extends ViewProps {
     'paperDirectEventDefinedInlineNullWithPaperName'
   > | null | undefined;
 
-  onBubblingEventDefinedInlineNull: BubblingEventHandler<null>;
-  onBubblingEventDefinedInlineNullOptionalKey?: BubblingEventHandler<null>;
-  onBubblingEventDefinedInlineNullOptionalValue: BubblingEventHandler<null> | null | undefined;
-  onBubblingEventDefinedInlineNullOptionalBoth?: BubblingEventHandler<null> | null | undefined;
+  onBubblingEventDefinedInlineNull: BubblingEventHandler<undefined>;
+  onBubblingEventDefinedInlineNullOptionalKey?: BubblingEventHandler<undefined>;
+  onBubblingEventDefinedInlineNullOptionalValue: BubblingEventHandler<undefined> | null | undefined;
+  onBubblingEventDefinedInlineNullOptionalBoth?: BubblingEventHandler<undefined> | null | undefined;
   onBubblingEventDefinedInlineNullWithPaperName?: BubblingEventHandler<
-    null,
+  undefined,
     'paperBubblingEventDefinedInlineNullWithPaperName'
   > | null | undefined;
 }

diff --git a/...rs/typescript/components/__tests__/__snapshots__/typescript-component-parser-test.js.snap b/...rs/typescript/components/__tests__/__snapshots__/typescript-component-parser-test.js.snap
@@ -14,7 +14,7 @@ exports[`RN Codegen TypeScript Parser Fails with error message COMMANDS_DEFINED_
 
 exports[`RN Codegen TypeScript Parser Fails with error message NON_OPTIONAL_KEY_WITH_DEFAULT_VALUE 1`] = `"key required_key_with_default must be optional if used with WithDefault<> annotation"`;
 
-exports[`RN Codegen TypeScript Parser Fails with error message NULLABLE_WITH_DEFAULT 1`] = `"WithDefault<> is optional and does not need to be marked as optional. Please remove the union of void and/or null"`;
+exports[`RN Codegen TypeScript Parser Fails with error message NULLABLE_WITH_DEFAULT 1`] = `"WithDefault<> is optional and does not need to be marked as optional. Please remove the union of undefined and/or null"`;
 
 exports[`RN Codegen TypeScript Parser Fails with error message PROP_ARRAY_ENUM_BOOLEAN 1`] = `"Unsupported union type for \\"someProp\\", received \\"BooleanLiteral\\""`;
 

diff --git a/packages/react-native-codegen/src/parsers/typescript/components/events.js b/packages/react-native-codegen/src/parsers/typescript/components/events.js
@@ -25,6 +25,9 @@ function getPropertyType(
    * LTI update could not be added via codemod */
   typeAnnotation,
 ): NamedShape<EventTypeAnnotation> {
+  if (typeAnnotation.type === 'TSParenthesizedType') {
+    return getPropertyType(name, optional, typeAnnotation.typeAnnotation);
+  }
   const type =
     typeAnnotation.type === 'TSTypeReference'
       ? typeAnnotation.typeName.name
@@ -100,10 +103,6 @@ function getPropertyType(
         )[0];
 
         // Check for <(T | T2) | null | undefined>
-        if (optionalType.type === 'TSParenthesizedType') {
-          return getPropertyType(name, true, optionalType.typeAnnotation);
-        }
-
         return getPropertyType(name, true, optionalType);
       }
 
@@ -144,19 +143,22 @@ function findEventArgumentsAndType(
         ? typeAnnotation.typeParameters.params[1].literal.value
         : null;
 
-    if (typeAnnotation.typeParameters.params[0].type === 'TSNullKeyword') {
-      return {
-        argumentProps: [],
-        bubblingType: eventType,
-        paperTopLevelNameDeprecated,
-      };
+    switch (typeAnnotation.typeParameters.params[0].type) {
+      case 'TSNullKeyword':
+      case 'TSUndefinedKeyword':
+        return {
+          argumentProps: [],
+          bubblingType: eventType,
+          paperTopLevelNameDeprecated,
+        };
+      default:
+        return findEventArgumentsAndType(
+          typeAnnotation.typeParameters.params[0],
+          types,
+          eventType,
+          paperTopLevelNameDeprecated,
+        );
     }
-    return findEventArgumentsAndType(
-      typeAnnotation.typeParameters.params[0],
-      types,
-      eventType,
-      paperTopLevelNameDeprecated,
-    );
   } else if (types[name]) {
     return findEventArgumentsAndType(
       types[name].typeAnnotation,
@@ -192,36 +194,49 @@ function getEventArgument(argumentProps, name: $FlowFixMe) {
   };
 }
 
+function findEvent(typeAnnotation: $FlowFixMe, optional: boolean) {
+  switch (typeAnnotation.type) {
+    // Check for T | null | undefined
+    case 'TSUnionType':
+      return findEvent(
+        typeAnnotation.types.filter(
+          t => t.type !== 'TSNullKeyword' && t.type !== 'TSUndefinedKeyword',
+        )[0],
+        optional ||
+          typeAnnotation.types.some(
+            t => t.type === 'TSNullKeyword' || t.type === 'TSUndefinedKeyword',
+          ),
+      );
+    // Check for (T)
+    case 'TSParenthesizedType':
+      return findEvent(typeAnnotation.typeAnnotation, optional);
+    case 'TSTypeReference':
+      if (
+        typeAnnotation.typeName.name !== 'BubblingEventHandler' &&
+        typeAnnotation.typeName.name !== 'DirectEventHandler'
+      ) {
+        return null;
+      } else {
+        return {typeAnnotation, optional};
+      }
+    default:
+      return null;
+  }
+}
+
 function buildEventSchema(
   types: TypeMap,
   property: EventTypeAST,
 ): ?EventTypeShape {
   const name = property.key.name;
-
-  let optional = property.optional || false;
-  let typeAnnotation = property.typeAnnotation.typeAnnotation;
-
-  // Check for T | null | undefined
-  if (
-    typeAnnotation.type === 'TSUnionType' &&
-    typeAnnotation.types.some(
-      t => t.type === 'TSNullKeyword' || t.type === 'TSUndefinedKeyword',
-    )
-  ) {
-    typeAnnotation = typeAnnotation.types.filter(
-      t => t.type !== 'TSNullKeyword' && t.type !== 'TSUndefinedKeyword',
-    )[0];
-    optional = true;
-  }
-
-  if (
-    typeAnnotation.type !== 'TSTypeReference' ||
-    (typeAnnotation.typeName.name !== 'BubblingEventHandler' &&
-      typeAnnotation.typeName.name !== 'DirectEventHandler')
-  ) {
+  const foundEvent = findEvent(
+    property.typeAnnotation.typeAnnotation,
+    property.optional || false,
+  );
+  if (!foundEvent) {
     return null;
   }
-
+  const {typeAnnotation, optional} = foundEvent;
   const {argumentProps, bubblingType, paperTopLevelNameDeprecated} =
     findEventArgumentsAndType(typeAnnotation, types);
 

diff --git a/packages/react-native-codegen/src/parsers/typescript/components/props.js b/packages/react-native-codegen/src/parsers/typescript/components/props.js
@@ -21,6 +21,11 @@ function getPropProperties(
   types: TypeDeclarationMap,
 ): $FlowFixMe {
   const alias = types[propsTypeName];
+  if (!alias) {
+    throw new Error(
+      `Failed to find definition for "${propsTypeName}", please check that you have a valid codegen typescript file`,
+    );
+  }
   const aliasKind =
     alias.type === 'TSInterfaceDeclaration' ? 'interface' : 'type';
 
@@ -255,6 +260,17 @@ function getTypeAnnotation(
 ) {
   const typeAnnotation = getValueFromTypes(annotation, types);
 
+  // Covers: (T)
+  if (typeAnnotation.type === 'TSParenthesizedType') {
+    return getTypeAnnotation(
+      name,
+      typeAnnotation.typeAnnotation,
+      defaultValue,
+      withNullDefault,
+      types,
+    );
+  }
+
   // Covers: readonly T[]
   if (
     typeAnnotation.type === 'TSTypeOperator' &&
@@ -464,6 +480,56 @@ function getTypeAnnotation(
   }
 }
 
+function findProp(
+  name: string,
+  typeAnnotation: $FlowFixMe,
+  optionalType: boolean,
+) {
+  switch (typeAnnotation.type) {
+    // Check for (T)
+    case 'TSParenthesizedType':
+      return findProp(name, typeAnnotation.typeAnnotation, optionalType);
+
+    // Check for optional type in union e.g. T | null | undefined
+    case 'TSUnionType':
+      return findProp(
+        name,
+        typeAnnotation.types.filter(
+          t => t.type !== 'TSNullKeyword' && t.type !== 'TSUndefinedKeyword',
+        )[0],
+        optionalType ||
+          typeAnnotation.types.some(
+            t => t.type === 'TSNullKeyword' || t.type === 'TSUndefinedKeyword',
+          ),
+      );
+
+    case 'TSTypeReference':
+      // Check against optional type inside `WithDefault`
+      if (typeAnnotation.typeName.name === 'WithDefault' && optionalType) {
+        throw new Error(
+          'WithDefault<> is optional and does not need to be marked as optional. Please remove the union of undefined and/or null',
+        );
+      }
+      // Remove unwanted types
+      if (
+        typeAnnotation.typeName.name === 'DirectEventHandler' ||
+        typeAnnotation.typeName.name === 'BubblingEventHandler'
+      ) {
+        return null;
+      }
+      if (
+        name === 'style' &&
+        typeAnnotation.type === 'GenericTypeAnnotation' &&
+        typeAnnotation.typeName.name === 'ViewStyleProp'
+      ) {
+        return null;
+      }
+      return {typeAnnotation, optionalType};
+    default:
+      return {typeAnnotation, optionalType};
+  }
+}
+
 function buildPropSchema(
   property: PropAST,
   types: TypeDeclarationMap,
@@ -475,39 +541,12 @@ function buildPropSchema(
     types,
   );
 
-  let typeAnnotation = value;
-  let optional = property.optional || false;
-
-  // Check for optional type in union e.g. T | null | undefined
-  if (
-    typeAnnotation.type === 'TSUnionType' &&
-    typeAnnotation.types.some(
-      t => t.type === 'TSNullKeyword' || t.type === 'TSUndefinedKeyword',
-    )
-  ) {
-    typeAnnotation = typeAnnotation.types.filter(
-      t => t.type !== 'TSNullKeyword' && t.type !== 'TSUndefinedKeyword',
-    )[0];
-    optional = true;
-
-    // Check against optional type inside `WithDefault`
-    if (
-      typeAnnotation.type === 'TSTypeReference' &&
-      typeAnnotation.typeName.name === 'WithDefault'
-    ) {
-      throw new Error(
-        'WithDefault<> is optional and does not need to be marked as optional. Please remove the union of void and/or null',
-      );
-    }
-  }
-
-  // example: WithDefault<string, ''>;
-  if (
-    value.type === 'TSTypeReference' &&
-    typeAnnotation.typeName.name === 'WithDefault'
-  ) {
-    optional = true;
+  const foundProp = findProp(name, value, false);
+  if (!foundProp) {
+    return null;
   }
+  let {typeAnnotation, optionalType} = foundProp;
+  let optional = property.optional || optionalType;
 
   // example: Readonly<{prop: string} | null | undefined>;
   if (
@@ -533,22 +572,6 @@ function buildPropSchema(
   }
 
   let type = typeAnnotation.type;
-  if (
-    type === 'TSTypeReference' &&
-    (typeAnnotation.typeName.name === 'DirectEventHandler' ||
-      typeAnnotation.typeName.name === 'BubblingEventHandler')
-  ) {
-    return null;
-  }
-
-  if (
-    name === 'style' &&
-    type === 'GenericTypeAnnotation' &&
-    typeAnnotation.typeName.name === 'ViewStyleProp'
-  ) {
-    return null;
-  }
-
   let defaultValue = null;
   let withNullDefault = false;
   if (
@@ -628,6 +651,7 @@ function flattenProperties(
         );
       }
     })
+    .filter(Boolean)
     .reduce((acc, item) => {
       if (Array.isArray(item)) {
         item.forEach(prop => {