Skip to content

Commit

Permalink
Fix codegen output for object with indexer (#35344)
Browse files Browse the repository at this point in the history 
Summary:
The current implementation think `{[key:T]:U}` and `{key:object}` are the same type, which is semantically wrong.

This pull request fixes the problem and return `{type:'GenericObjectTypeAnnotation'}`, so that `{[key:T]:U}` is `Object`. The current schema cannot represent dictionary type, `Object` is the closest one.

The previous incorrect implementation actually bring in code with logic that doesn't make sense (treating indexer as property), those and related unit test are all undone.

## Changelog

[General] [Changed] - Fix codegen output for object with indexer

Pull Request resolved: #35344

Test Plan: `yarn jest react-native-codegen` passed

Reviewed By: rshest

Differential Revision: D41304475

Pulled By: cipolleschi

fbshipit-source-id: caab8e458d83f9850c5c28b67cc561a764738372
  • Loading branch information
@ZihanChen-MSFT @facebook-github-bot
ZihanChen-MSFT authored and facebook-github-bot committed Dec 13, 2022
1 parent 9b280ad commit f07490b
Show file tree
Hide file tree
Showing 12 changed files with 123 additions and 274 deletions.
65 changes: 4 additions & 61 deletions packages/react-native-codegen/src/parsers/__tests__/parsers-commons-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,18 +225,7 @@ describe('isObjectProperty', () => {
expect(result).toEqual(true);
});

it("returns 'true' if 'property.type' is 'ObjectTypeIndexer'", () => {
const result = isObjectProperty(
{
type: 'ObjectTypeIndexer',
...propertyStub,
},
language,
);
expect(result).toEqual(true);
});

it("returns 'false' if 'property.type' is not 'ObjectTypeProperty' or 'ObjectTypeIndexer'", () => {
it("returns 'false' if 'property.type' is not 'ObjectTypeProperty'", () => {
const result = isObjectProperty(
{
type: 'notObjectTypeProperty',
Expand All @@ -261,18 +250,7 @@ describe('isObjectProperty', () => {
expect(result).toEqual(true);
});

it("returns 'true' if 'property.type' is 'TSIndexSignature'", () => {
const result = isObjectProperty(
{
type: 'TSIndexSignature',
...propertyStub,
},
language,
);
expect(result).toEqual(true);
});

it("returns 'false' if 'property.type' is not 'TSPropertySignature' or 'TSIndexSignature'", () => {
it("returns 'false' if 'property.type' is not 'TSPropertySignature'", () => {
const result = isObjectProperty(
{
type: 'notTSPropertySignature',
Expand All @@ -295,7 +273,7 @@ describe('parseObjectProperty', () => {

describe("when 'language' is 'Flow'", () => {
const language: ParserType = 'Flow';
it("throws an 'UnsupportedObjectPropertyTypeAnnotationParserError' error if 'property.type' is not 'ObjectTypeProperty' or 'ObjectTypeIndexer'.", () => {
it("throws an 'UnsupportedObjectPropertyTypeAnnotationParserError' error if 'property.type' is not 'ObjectTypeProperty'.", () => {
const property = {
type: 'notObjectTypeProperty',
typeAnnotation: {
Expand Down Expand Up @@ -329,7 +307,7 @@ describe('parseObjectProperty', () => {

describe("when 'language' is 'TypeScript'", () => {
const language: ParserType = 'TypeScript';
it("throws an 'UnsupportedObjectPropertyTypeAnnotationParserError' error if 'property.type' is not 'TSPropertySignature' or 'TSIndexSignature'.", () => {
it("throws an 'UnsupportedObjectPropertyTypeAnnotationParserError' error if 'property.type' is not 'TSPropertySignature'.", () => {
const property = {
type: 'notTSPropertySignature',
typeAnnotation: {
Expand Down Expand Up @@ -358,40 +336,5 @@ describe('parseObjectProperty', () => {
),
).toThrow(expected);
});

it("returns a 'NativeModuleBaseTypeAnnotation' object with 'typeAnnotation.type' equal to 'GenericObjectTypeAnnotation', if 'property.type' is 'TSIndexSignature'.", () => {
const property = {
type: 'TSIndexSignature',
typeAnnotation: {
type: 'TSIndexSignature',
typeAnnotation: 'TSIndexSignature',
},
key: {
name: 'testKeyName',
},
value: 'wrongValue',
name: 'wrongName',
parameters: [{name: 'testName'}],
};
const result = parseObjectProperty(
property,
moduleName,
types,
aliasMap,
tryParse,
cxxOnly,
nullable,
typeScriptTranslateTypeAnnotation,
typeScriptParser,
);
const expected = {
name: 'testName',
optional: false,
typeAnnotation: wrapNullable(nullable, {
type: 'GenericObjectTypeAnnotation',
}),
};
expect(result).toEqual(expected);
});
});
});
43 changes: 0 additions & 43 deletions packages/react-native-codegen/src/parsers/__tests__/parsers-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,32 +37,6 @@ describe('FlowParser', () => {
});
});

describe('when propertyOrIndex is ObjectTypeIndexer', () => {
it('returns indexer name', () => {
const indexer = {
type: 'ObjectTypeIndexer',
id: {
name: 'indexerName',
},
};

const expected = 'indexerName';

expect(parser.getKeyName(indexer, hasteModuleName)).toEqual(expected);
});

it('returns `key` if indexer has no name', () => {
const indexer = {
type: 'ObjectTypeIndexer',
id: {},
};

const expected = 'key';

expect(parser.getKeyName(indexer, hasteModuleName)).toEqual(expected);
});
});

describe('when propertyOrIndex is not ObjectTypeProperty or ObjectTypeIndexer', () => {
it('throw UnsupportedObjectPropertyTypeAnnotationParserError', () => {
const indexer = {
Expand Down Expand Up @@ -113,23 +87,6 @@ describe('TypeScriptParser', () => {
});
});

describe('when propertyOrIndex is TSIndexSignature', () => {
it('returns indexer name', () => {
const indexer = {
type: 'TSIndexSignature',
parameters: [
{
name: 'indexerName',
},
],
};

const expected = 'indexerName';

expect(parser.getKeyName(indexer, hasteModuleName)).toEqual(expected);
});
});

describe('when propertyOrIndex is not TSPropertySignature or TSIndexSignature', () => {
it('throw UnsupportedObjectPropertyTypeAnnotationParserError', () => {
const indexer = {
Expand Down
3 changes: 2 additions & 1 deletion packages/react-native-codegen/src/parsers/flow/modules/__test_fixtures__/fixtures.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,8 @@ export interface Spec extends TurboModule {
returnObjectArray(): Promise<Array<Object>>;
returnNullableNumber(): Promise<number | null>;
returnEmpty(): Promise<empty>;
returnIndex(): Promise<{ [string]: 'authorized' | 'denied' | 'undetermined' | true | false }>;
returnUnsupportedIndex(): Promise<{ [string]: 'authorized' | 'denied' | 'undetermined' | true | false }>;
returnSupportedIndex(): Promise<{ [string]: CustomObject }>;
returnEnum() : Promise<Season>;
returnObject() : Promise<CustomObject>;
}
Expand Down
68 changes: 17 additions & 51 deletions ...egen/src/parsers/flow/modules/__tests__/__snapshots__/module-parser-snapshot-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,32 +126,14 @@ exports[`RN Codegen Flow Parser can generate fixture CXX_ONLY_NATIVE_MODULE 1`]
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'ObjectTypeAnnotation',
'properties': [
{
'name': 'b',
'optional': false,
'typeAnnotation': {
'type': 'GenericObjectTypeAnnotation'
}
}
]
'type': 'GenericObjectTypeAnnotation'
},
'params': [
{
'name': 'arg',
'optional': false,
'typeAnnotation': {
'type': 'ObjectTypeAnnotation',
'properties': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'GenericObjectTypeAnnotation'
}
}
]
'type': 'GenericObjectTypeAnnotation'
}
}
]
Expand All @@ -163,32 +145,14 @@ exports[`RN Codegen Flow Parser can generate fixture CXX_ONLY_NATIVE_MODULE 1`]
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'ObjectTypeAnnotation',
'properties': [
{
'name': 'key',
'optional': false,
'typeAnnotation': {
'type': 'GenericObjectTypeAnnotation'
}
}
]
'type': 'GenericObjectTypeAnnotation'
},
'params': [
{
'name': 'arg',
'optional': false,
'typeAnnotation': {
'type': 'ObjectTypeAnnotation',
'properties': [
{
'name': 'key',
'optional': false,
'typeAnnotation': {
'type': 'GenericObjectTypeAnnotation'
}
}
]
'type': 'GenericObjectTypeAnnotation'
}
}
]
Expand Down Expand Up @@ -1810,23 +1774,25 @@ exports[`RN Codegen Flow Parser can generate fixture PROMISE_WITH_COMMONLY_USED_
}
},
{
'name': 'returnIndex',
'name': 'returnUnsupportedIndex',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'PromiseTypeAnnotation'
},
'params': []
}
},
{
'name': 'returnSupportedIndex',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'PromiseTypeAnnotation',
'elementType': {
'type': 'ObjectTypeAnnotation',
'properties': [
{
'name': 'key',
'optional': false,
'typeAnnotation': {
'type': 'GenericObjectTypeAnnotation'
}
}
]
'type': 'GenericObjectTypeAnnotation'
}
},
'params': []
Expand Down
25 changes: 24 additions & 1 deletion packages/react-native-codegen/src/parsers/flow/modules/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,28 @@ function translateTypeAnnotation(
}
}
case 'ObjectTypeAnnotation': {
// if there is any indexer, then it is a dictionary
if (typeAnnotation.indexers) {
const indexers = typeAnnotation.indexers.filter(
member => member.type === 'ObjectTypeIndexer',
);
if (indexers.length > 0) {
// check the property type to prevent developers from using unsupported types
// the return value from `translateTypeAnnotation` is unused
const propertyType = indexers[0].value;
translateTypeAnnotation(
hasteModuleName,
propertyType,
types,
aliasMap,
tryParse,
cxxOnly,
);
// no need to do further checking
return emitObject(nullable);
}
}

const objectTypeAnnotation = {
type: 'ObjectTypeAnnotation',
// $FlowFixMe[missing-type-arg]
Expand Down Expand Up @@ -238,8 +260,9 @@ function translateTypeAnnotation(
case 'MixedTypeAnnotation': {
if (cxxOnly) {
return emitMixed(nullable);
} else {
return emitObject(nullable);
}
// Fallthrough
}
default: {
throw new UnsupportedTypeAnnotationParserError(
Expand Down
27 changes: 13 additions & 14 deletions packages/react-native-codegen/src/parsers/flow/parser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,20 @@ const {
class FlowParser implements Parser {
typeParameterInstantiation: string = 'TypeParameterInstantiation';

getKeyName(propertyOrIndex: $FlowFixMe, hasteModuleName: string): string {
switch (propertyOrIndex.type) {
case 'ObjectTypeProperty':
return propertyOrIndex.key.name;
case 'ObjectTypeIndexer':
// flow index name is optional
return propertyOrIndex.id?.name ?? 'key';
default:
throw new UnsupportedObjectPropertyTypeAnnotationParserError(
hasteModuleName,
propertyOrIndex,
propertyOrIndex.type,
this.language(),
);
isProperty(property: $FlowFixMe): boolean {
return property.type === 'ObjectTypeProperty';
}

getKeyName(property: $FlowFixMe, hasteModuleName: string): string {
if (!this.isProperty(property)) {
throw new UnsupportedObjectPropertyTypeAnnotationParserError(
hasteModuleName,
property,
property.type,
this.language(),
);
}
return property.key.name;
}

getMaybeEnumMemberType(maybeEnumDeclaration: $FlowFixMe): string {
Expand Down
12 changes: 8 additions & 4 deletions packages/react-native-codegen/src/parsers/parser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,17 @@ export interface Parser {
typeParameterInstantiation: string;

/**
* Given a property or an index declaration, it returns the key name.
* @parameter propertyOrIndex: an object containing a property or an index declaration.
* Given a declaration, it returns true if it is a property
*/
isProperty(property: $FlowFixMe): boolean;
/**
* Given a property declaration, it returns the key name.
* @parameter property: an object containing a property declaration.
* @parameter hasteModuleName: a string with the native module name.
* @returns: the key name.
* @throws if propertyOrIndex does not contain a property or an index declaration.
* @throws if property does not contain a property declaration.
*/
getKeyName(propertyOrIndex: $FlowFixMe, hasteModuleName: string): string;
getKeyName(property: $FlowFixMe, hasteModuleName: string): string;
/**
* Given a type declaration, it possibly returns the name of the Enum type.
* @parameter maybeEnumDeclaration: an object possibly containing an Enum declaration.
Expand Down

0 comments on commit f07490b

Please sign in to comment.