Skip to content

Commit

Permalink
Fix nullability lost on readonly types
Browse files Browse the repository at this point in the history 
Summary:
This fixes an issue where nullability was lost on readonly types in TurboModules specs. The "outside" nullability would be ignored in favor of only respecting the "inside" nullability (i.e. `?$ReadOnly<{}>` vs `$ReadOnly<?{}>`). It now ensure either is propagated correctly.

Changelog:
[General][Fixed] Fix nullability lost on readonly types in TurboModule specs

Reviewed By: RSNara

Differential Revision: D36615346

fbshipit-source-id: 42408d9298703f4c382657f573480d2574d6c973
  • Loading branch information
@appden @facebook-github-bot
appden authored and facebook-github-bot committed May 25, 2022
1 parent 3c569f5 commit c006722
Show file tree
Hide file tree
Showing 9 changed files with 216 additions and 16 deletions.
41 changes: 40 additions & 1 deletion 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 @@ -1506,7 +1506,20 @@ const CXX_ONLY_NATIVE_MODULES: SchemaType = {
modules: {
NativeSampleTurboModule: {
type: 'NativeModule',
aliases: {},
aliases: {
ObjectAlias: {
type: 'ObjectTypeAnnotation',
properties: [
{
name: 'x',
optional: false,
typeAnnotation: {
type: 'NumberTypeAnnotation',
},
},
],
},
},
spec: {
properties: [
{
Expand All @@ -1528,6 +1541,32 @@ const CXX_ONLY_NATIVE_MODULES: SchemaType = {
],
},
},
{
name: 'getNullableNumberFromNullableAlias',
optional: false,
typeAnnotation: {
type: 'FunctionTypeAnnotation',
returnTypeAnnotation: {
type: 'NullableTypeAnnotation',
typeAnnotation: {
type: 'NumberTypeAnnotation',
},
},
params: [
{
name: 'a',
optional: false,
typeAnnotation: {
type: 'NullableTypeAnnotation',
typeAnnotation: {
type: 'TypeAliasTypeAnnotation',
name: 'ObjectAlias',
},
},
},
],
},
},
],
},
moduleNames: ['SampleTurboModuleCxx'],
Expand Down
4 changes: 4 additions & 0 deletions ...ive-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleCpp-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,14 @@ namespace react {
static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getMixed(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
return static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->getMixed(rt, jsi::Value(rt, args[0]));
}
static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getNullableNumberFromNullableAlias(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
return static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->getNullableNumberFromNullableAlias(rt, args[0].isNull() || args[0].isUndefined() ? std::nullopt : std::make_optional(args[0].asObject(rt)));
}
NativeSampleTurboModuleCxxSpecJSI::NativeSampleTurboModuleCxxSpecJSI(std::shared_ptr<CallInvoker> jsInvoker)
: TurboModule(\\"SampleTurboModuleCxx\\", jsInvoker) {
methodMap_[\\"getMixed\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getMixed};
methodMap_[\\"getNullableNumberFromNullableAlias\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getNullableNumberFromNullableAlias};
}
Expand Down
9 changes: 9 additions & 0 deletions ...ative-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleH-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ protected:
public:
virtual jsi::Value getMixed(jsi::Runtime &rt, jsi::Value arg) = 0;
virtual std::optional<double> getNullableNumberFromNullableAlias(jsi::Runtime &rt, std::optional<jsi::Object> a) = 0;
};
Expand Down Expand Up @@ -237,6 +238,14 @@ private:
return bridging::callFromJs<jsi::Value>(
rt, &T::getMixed, jsInvoker_, instance_, std::move(arg));
}
std::optional<double> getNullableNumberFromNullableAlias(jsi::Runtime &rt, std::optional<jsi::Object> a) override {
static_assert(
bridging::getParameterCount(&T::getNullableNumberFromNullableAlias) == 2,
\\"Expected getNullableNumberFromNullableAlias(...) to have 2 parameters\\");
return bridging::callFromJs<std::optional<double>>(
rt, &T::getNullableNumberFromNullableAlias, jsInvoker_, instance_, std::move(a));
}
private:
T *instance_;
Expand Down
6 changes: 5 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 @@ -161,14 +161,18 @@ export type ObjectAlias = {|
y: number,
label: string,
truthy: boolean,
|}
|};
export type ReadOnlyAlias = $ReadOnly<ObjectAlias>;
export interface Spec extends TurboModule {
// Exported methods.
+getNumber: Num2;
+getVoid: () => Void;
+getArray: (a: Array<A>) => {| a: B |};
+getStringFromAlias: (a: ObjectAlias) => string;
+getStringFromNullableAlias: (a: ?ObjectAlias) => string;
+getStringFromReadOnlyAlias: (a: ReadOnlyAlias) => string;
+getStringFromNullableReadOnlyAlias: (a: ?ReadOnlyAlias) => string;
}
export default TurboModuleRegistry.getEnforcing<Spec>('SampleTurboModule');
Expand Down
66 changes: 66 additions & 0 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 @@ -251,6 +251,72 @@ exports[`RN Codegen Flow Parser can generate fixture NATIVE_MODULE_WITH_ALIASES
}
]
}
},
{
'name': 'getStringFromNullableAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'NullableTypeAnnotation',
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
}
]
}
},
{
'name': 'getStringFromReadOnlyAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
]
}
},
{
'name': 'getStringFromNullableReadOnlyAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'NullableTypeAnnotation',
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
}
]
}
}
]
},
Expand Down
18 changes: 11 additions & 7 deletions packages/react-native-codegen/src/parsers/flow/modules/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,18 @@ function translateTypeAnnotation(
typeAnnotation,
);

return translateTypeAnnotation(
hasteModuleName,
typeAnnotation.typeParameters.params[0],
types,
aliasMap,
tryParse,
cxxOnly,
const [paramType, isParamNullable] = unwrapNullable(
translateTypeAnnotation(
hasteModuleName,
typeAnnotation.typeParameters.params[0],
types,
aliasMap,
tryParse,
cxxOnly,
),
);

return wrapNullable(nullable || isParamNullable, paramType);
}
case 'Stringish': {
return wrapNullable(nullable, {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-native-codegen/src/parsers/typescript/modules/__test_fixtures__/fixtures.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,17 @@ export type ObjectAlias = {
label: string;
truthy: boolean;
};
export type ReadOnlyAlias = Readonly<ObjectAlias>;
export interface Spec extends TurboModule {
// Exported methods.
readonly getNumber: Num2;
readonly getVoid: () => Void;
readonly getArray: (a: Array<A>) => {a: B};
readonly getStringFromAlias: (a: ObjectAlias) => string;
readonly getStringFromNullableAlias: (a: ObjectAlias | null) => string;
readonly getStringFromReadOnlyAlias: (a: ReadOnlyAlias) => string;
readonly getStringFromNullableReadOnlyAlias: (a: ReadOnlyAlias | null) => string;
}
export default TurboModuleRegistry.getEnforcing<Spec>('SampleTurboModule');
Expand Down
66 changes: 66 additions & 0 deletions ...typescript/modules/__tests__/__snapshots__/typescript-module-parser-snapshot-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,72 @@ exports[`RN Codegen TypeScript Parser can generate fixture NATIVE_MODULE_WITH_AL
}
]
}
},
{
'name': 'getStringFromNullableAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'NullableTypeAnnotation',
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
}
]
}
},
{
'name': 'getStringFromReadOnlyAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
]
}
},
{
'name': 'getStringFromNullableReadOnlyAlias',
'optional': false,
'typeAnnotation': {
'type': 'FunctionTypeAnnotation',
'returnTypeAnnotation': {
'type': 'StringTypeAnnotation'
},
'params': [
{
'name': 'a',
'optional': false,
'typeAnnotation': {
'type': 'NullableTypeAnnotation',
'typeAnnotation': {
'type': 'TypeAliasTypeAnnotation',
'name': 'ObjectAlias'
}
}
}
]
}
}
]
},
Expand Down
18 changes: 11 additions & 7 deletions packages/react-native-codegen/src/parsers/typescript/modules/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,18 @@ function translateTypeAnnotation(
typeAnnotation,
);

return translateTypeAnnotation(
hasteModuleName,
typeAnnotation.typeParameters.params[0],
types,
aliasMap,
tryParse,
cxxOnly,
const [paramType, isParamNullable] = unwrapNullable(
translateTypeAnnotation(
hasteModuleName,
typeAnnotation.typeParameters.params[0],
types,
aliasMap,
tryParse,
cxxOnly,
),
);

return wrapNullable(nullable || isParamNullable, paramType);
}
case 'Stringish': {
return wrapNullable(nullable, {
Expand Down

0 comments on commit c006722

Please sign in to comment.