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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5992fa
Handle (T) and optional in turbo module component codegen
ZihanChen-MSFT Sep 14, 2022
4347a2a
yarn prettier
ZihanChen-MSFT Sep 14, 2022
af10a51
Update props.js
ZihanChen-MSFT Sep 14, 2022
c40b411
Fix typing
ZihanChen-MSFT Sep 14, 2022
fa8137d
Update props.js
ZihanChen-MSFT Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions ...ages/react-native-codegen/src/parsers/typescript/components/__test_fixtures__/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this probably does not change anything, but I think we should keep also the other syntax, without parenthesis, to make sure that all the use cases are covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, if you scroll down the file you will see a lot of similar thing, which remains unchanged. So cases without parenthesis are still covered.


double_required: Double;
double_optional_key?: Double;
Expand Down Expand Up @@ -764,23 +764,23 @@ export interface ModuleProps extends ViewProps {
}>
>;

onDirectEventDefinedInlineOptionalKey?: DirectEventHandler<
onDirectEventDefinedInlineOptionalKey?: (DirectEventHandler<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BubblingEventHandler cases below still covered syntax without parenthesis.

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<{
Expand Down Expand Up @@ -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<
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
null,
undefined,
'paperBubblingEventDefinedInlineNullWithPaperName'
> | null | undefined;
}
Expand Down
2 changes: 1 addition & 1 deletion ...rs/typescript/components/__tests__/__snapshots__/typescript-component-parser-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\\""`;

Expand Down
93 changes: 54 additions & 39 deletions packages/react-native-codegen/src/parsers/typescript/components/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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'
ZihanChen-MSFT marked this conversation as resolved.
Show resolved Hide resolved
) {
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);

Expand Down
120 changes: 72 additions & 48 deletions packages/react-native-codegen/src/parsers/typescript/components/props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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' &&
Expand Down Expand Up @@ -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'
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
) {
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,
Expand All @@ -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 (
Expand All @@ -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 (
Expand Down Expand Up @@ -628,6 +651,7 @@ function flattenProperties(
);
}
})
.filter(Boolean)
.reduce((acc, item) => {
if (Array.isArray(item)) {
item.forEach(prop => {
Expand Down