Skip to content

Commit

Permalink
Handle mixed union types and arrays of objects
Browse files Browse the repository at this point in the history 
Summary:
Restore legacy support for mixed union types.
These are not type safe and modules should use a different type, but we have a precedent for supporting these in the existing Linking native module. The new codegen will generate native code for these, and show a warning to encourage use of a better type.

Generate native code for elements in arrays of objects.

Changelog: [Internal]

Reviewed By: RSNara

Differential Revision: D21848260

fbshipit-source-id: 0b8cbf25e7a02791b4d77e349227a2b0744854f4
  • Loading branch information
@hramos @facebook-github-bot
hramos authored and facebook-github-bot committed Jun 10, 2020
1 parent fa5d3fb commit 1c92b1c
Show file tree
Hide file tree
Showing 11 changed files with 186 additions and 14 deletions.
2 changes: 1 addition & 1 deletion Libraries/Alert/NativeAlertManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import * as TurboModuleRegistry from '../TurboModule/TurboModuleRegistry';
export type Args = {|
title?: string,
message?: string,
buttons?: Array<Object>, // TODO: have a better type
buttons?: Array<Object>, // TODO(T67565166): have a better type
type?: string,
defaultValue?: string,
cancelButtonKey?: string,
Expand Down
1 change: 0 additions & 1 deletion Libraries/Core/NativeExceptionsManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export interface Spec extends TurboModule {
stack: Array<StackFrame>,
exceptionId: number,
) => void;
// TODO(T53311281): This is a noop on iOS now. Implement it.
+reportException?: (data: ExceptionData) => void;
+updateExceptionMessage: (
message: string,
Expand Down
2 changes: 1 addition & 1 deletion Libraries/Linking/NativeLinking.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export interface Spec extends TurboModule {
action: string,
extras: ?Array<{
key: string,
value: string | number | boolean,
value: string | number | boolean, // TODO(T67672788): Union types are not type safe
...
}>,
) => Promise<void>;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native-codegen/src/CodegenSchema.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export type FunctionTypeAnnotationReturnArrayElementType = FunctionTypeAnnotatio
export type ObjectParamTypeAnnotation = $ReadOnly<{|
optional: boolean,
name: string,
typeAnnotation: FunctionTypeAnnotationParamTypeAnnotation,
typeAnnotation?: FunctionTypeAnnotationParamTypeAnnotation, // TODO (T67898313): Workaround for NativeLinking's use of union type, typeAnnotations should not be optional
|}>;

export type FunctionTypeAnnotationReturn =
Expand Down
46 changes: 45 additions & 1 deletion packages/react-native-codegen/src/generators/modules/GenerateModuleHObjCpp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,31 @@ function translatePrimitiveJSTypeToObjCTypeForReturn(
}
}

function handleArrayOfObjects(
objectForGeneratingStructs: Array<ObjectForGeneratingStructs>,
param: FunctionTypeAnnotationParam,
name: string,
) {
if (
param.typeAnnotation.type === 'ArrayTypeAnnotation' &&
param.typeAnnotation.elementType &&
param.typeAnnotation.elementType.type === 'ObjectTypeAnnotation' &&
param.typeAnnotation.elementType.properties
) {
const {elementType} = param.typeAnnotation;
const {properties} = elementType;
if (properties && properties.length > 0) {
objectForGeneratingStructs.push({
name,
object: {
type: 'ObjectTypeAnnotation',
properties,
},
});
}
}
}

const methodImplementationTemplate =
'- (::_RETURN_VALUE_::) ::_PROPERTY_NAME_::::_ARGS_::;';

Expand Down Expand Up @@ -232,12 +257,31 @@ module.exports = {
},
});
paramObjCType = `JS::Native::_MODULE_NAME_::::Spec${variableName}&`;

param.typeAnnotation.properties.map(aProp =>
handleArrayOfObjects(
objectForGeneratingStructs,
aProp,
capitalizeFirstLetter(prop.name) +
capitalizeFirstLetter(param.name) +
capitalizeFirstLetter(aProp.name) +
'Element',
),
);
} else {
paramObjCType = translatePrimitiveJSTypeToObjCType(
param,
typeName =>
`Unsupported type for param "${param.name}" in ${prop.name}. Found: ${typeName}`,
);

handleArrayOfObjects(
objectForGeneratingStructs,
param,
capitalizeFirstLetter(prop.name) +
capitalizeFirstLetter(param.name) +
'Element',
);
}
return `${i === 0 ? '' : param.name}:(${paramObjCType})${
param.name
Expand Down Expand Up @@ -285,7 +329,7 @@ module.exports = {
return protocolTemplate
.replace(
/::_STRUCTS_::/g,
translateObjectsForStructs(objectForGeneratingStructs),
translateObjectsForStructs(objectForGeneratingStructs, name),
)
.replace(/::_MODULE_PROPERTIES_::/g, implementations)
.replace(/::_MODULE_NAME_::/g, name)
Expand Down
62 changes: 57 additions & 5 deletions packages/react-native-codegen/src/generators/modules/ObjCppUtils/GenerateStructs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,17 @@ function getNamespacedStructName(structName: string, propertyName: string) {
function getElementTypeForArray(
property: ObjectParamTypeAnnotation,
name: string,
moduleName: string,
): string {
const {typeAnnotation} = property;

// TODO(T67898313): Workaround for NativeLinking's use of union type. This check may be removed once typeAnnotation is non-optional.
if (!typeAnnotation) {
throw new Error(
`Cannot get array element type, property ${property.name} does not contain a type annotation`,
);
}

if (typeAnnotation.type !== 'ArrayTypeAnnotation') {
throw new Error(
`Cannot get array element type for non-array type ${typeAnnotation.type}`,
Expand All @@ -82,9 +91,12 @@ function getElementTypeForArray(
case 'Int32TypeAnnotation':
return 'double';
case 'ObjectTypeAnnotation':
return getNamespacedStructName(name, property.name);
return getNamespacedStructName(name, property.name) + 'Element';
case 'GenericObjectTypeAnnotation':
// TODO T67565166: Generic objects are not type safe and should be disallowed in the schema.
// TODO(T67565166): Generic objects are not type safe and should be disallowed in the schema. This case should throw an error once it is disallowed in schema.
console.error(
`Warning: Generic objects are not type safe and should be avoided whenever possible (see '${property.name}' in ${moduleName}'s ${name})`,
);
return 'id<NSObject>';
case 'BooleanTypeAnnotation':
case 'AnyObjectTypeAnnotation':
Expand All @@ -104,6 +116,7 @@ function getElementTypeForArray(
function getInlineMethodSignature(
property: ObjectParamTypeAnnotation,
name: string,
moduleName: string,
): string {
const {typeAnnotation} = property;
function markOptionalTypeIfNecessary(type: string) {
Expand All @@ -112,6 +125,15 @@ function getInlineMethodSignature(
}
return type;
}

// TODO(T67672788): Workaround for values key in NativeLinking which lacks a typeAnnotation. id<NSObject> is not type safe!
if (!typeAnnotation) {
console.error(
`Warning: Unsafe type found (see '${property.name}' in ${moduleName}'s ${name})`,
);
return `id<NSObject> ${getSafePropertyName(property.name)}() const;`;
}

switch (typeAnnotation.type) {
case 'ReservedFunctionValueTypeAnnotation':
switch (typeAnnotation.name) {
Expand Down Expand Up @@ -149,6 +171,7 @@ function getInlineMethodSignature(
`facebook::react::LazyVector<${getElementTypeForArray(
property,
name,
moduleName,
)}>`,
)} ${getSafePropertyName(property.name)}() const;`;
case 'FunctionTypeAnnotation':
Expand All @@ -160,6 +183,7 @@ function getInlineMethodSignature(
function getInlineMethodImplementation(
property: ObjectParamTypeAnnotation,
name: string,
moduleName: string,
): string {
const {typeAnnotation} = property;
function markOptionalTypeIfNecessary(type: string): string {
Expand All @@ -175,6 +199,13 @@ function getInlineMethodImplementation(
return `RCTBridgingTo${capitalizeFirstLetter(value)}`;
}
function bridgeArrayElementValueIfNecessary(element: string): string {
// TODO(T67898313): Workaround for NativeLinking's use of union type
if (!typeAnnotation) {
throw new Error(
`Cannot get array element type, property ${property.name} does not contain a type annotation`,
);
}

if (typeAnnotation.type !== 'ArrayTypeAnnotation') {
throw new Error(
`Cannot get array element type for non-array type ${typeAnnotation.type}`,
Expand All @@ -197,7 +228,10 @@ function getInlineMethodImplementation(
case 'BooleanTypeAnnotation':
return `RCTBridgingToBool(${element})`;
case 'ObjectTypeAnnotation':
return `${getNamespacedStructName(name, property.name)}(${element})`;
return `${getNamespacedStructName(
name,
property.name,
)}Element(${element})`;
case 'GenericObjectTypeAnnotation':
return element;
case 'AnyObjectTypeAnnotation':
Expand All @@ -215,6 +249,19 @@ function getInlineMethodImplementation(
}
}

// TODO(T67672788): Workaround for values key in NativeLinking which lacks a typeAnnotation. id<NSObject> is not type safe!
if (!typeAnnotation) {
console.error(
`Warning: Unsafe type found (see '${property.name}' in ${moduleName}'s ${name})`,
);
return inlineTemplate
.replace(
/::_RETURN_TYPE_::/,
property.optional ? 'id<NSObject> _Nullable ' : 'id<NSObject> ',
)
.replace(/::_RETURN_VALUE_::/, 'p');
}

switch (typeAnnotation.type) {
case 'ReservedFunctionValueTypeAnnotation':
switch (typeAnnotation.name) {
Expand Down Expand Up @@ -279,6 +326,7 @@ function getInlineMethodImplementation(
`facebook::react::LazyVector<${getElementTypeForArray(
property,
name,
moduleName,
)}>`,
),
)
Expand All @@ -287,6 +335,7 @@ function getInlineMethodImplementation(
`${markOptionalValueIfNecessary('vec')}(p, ^${getElementTypeForArray(
property,
name,
moduleName,
)}(id itemValue_0) { return ${bridgeArrayElementValueIfNecessary(
'itemValue_0',
)}; })`,
Expand All @@ -307,6 +356,7 @@ function translateObjectsForStructs(
|}>,
|}>,
>,
moduleName: string,
): string {
const flattenObjects = flatObjects(annotations);

Expand All @@ -315,7 +365,7 @@ function translateObjectsForStructs(
(acc, object) =>
acc.concat(
object.properties.map(property =>
getInlineMethodImplementation(property, object.name)
getInlineMethodImplementation(property, object.name, moduleName)
.replace(
/::_PROPERTY_NAME_::/g,
getSafePropertyName(property.name),
Expand All @@ -333,7 +383,9 @@ function translateObjectsForStructs(
.replace(
/::_STRUCT_PROPERTIES_::/g,
object.properties
.map(property => getInlineMethodSignature(property, object.name))
.map(property =>
getInlineMethodSignature(property, object.name, moduleName),
)
.join('\n '),
)
.replace(/::_STRUCT_NAME_::/g, object.name),
Expand Down
16 changes: 16 additions & 0 deletions ...es/react-native-codegen/src/generators/modules/ObjCppUtils/GenerateStructsForConstants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ function getBuilderInputFieldDeclaration(
return 'folly::Optional<' + annotation + '> ' + property.name + ';';
}
const {typeAnnotation} = property;

// TODO(T67898313): Workaround for NativeLinking's use of union type. This check may be removed once typeAnnotation is non-optional.
if (!typeAnnotation) {
throw new Error(
`Cannot get array element type, property ${property.name} does not contain a type annotation`,
);
}

switch (typeAnnotation.type) {
case 'ReservedFunctionValueTypeAnnotation':
switch (typeAnnotation.name) {
Expand Down Expand Up @@ -153,6 +161,14 @@ function unsafeGetter(name: string, optional: boolean) {

function getObjectProperty(property: ObjectParamTypeAnnotation): string {
const {typeAnnotation} = property;

// TODO(T67898313): Workaround for NativeLinking's use of union type. This check may be removed once typeAnnotation is non-optional.
if (!typeAnnotation) {
throw new Error(
`Cannot get array element type, property ${property.name} does not contain a type annotation`,
);
}

switch (typeAnnotation.type) {
case 'ReservedFunctionValueTypeAnnotation':
switch (typeAnnotation.name) {
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-codegen/src/generators/modules/ObjCppUtils/Utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function flatObjects(
objectTypesToFlatten = objectTypesToFlatten.concat(
properties.reduce((acc, curr) => {
if (
curr.typeAnnotation &&
curr.typeAnnotation.type === 'ObjectTypeAnnotation' &&
curr.typeAnnotation.properties
) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-native-codegen/src/generators/modules/__test_fixtures__/fixtures.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,10 @@ const COMPLEX_OBJECTS: SchemaType = {
type: 'StringTypeAnnotation',
},
},
{
optional: false,
name: 'value',
},
],
},
},
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native-codegen/src/generators/modules/__tests__/GenerateStructs-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('GenerateStructs', () => {
it(`can generate fixture ${fixtureName}`, () => {
expect(
generator
.translateObjectsForStructs(fixture)
.translateObjectsForStructs(fixture, fixtureName)
.replace(/::_MODULE_NAME_::/g, 'SampleTurboModule'),
).toMatchSnapshot();
});
Expand Down
Loading

0 comments on commit 1c92b1c

Please sign in to comment.