Fix codegen to add T of Promise<T> in CodegenSchema.js

[ZihanChen-MSFT]

Summary

Promise<T> is used very often in turbo module as function return types. So T is a critical thing for type safety. To enable future generator to produce more specific type for Promise, T is added to the schema.

Changelog

[General] [Changed] - Fix codegen to add T of Promise<T> in CodegenSchema.js

Test Plan

yarn jest react-native-codegen passed

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 6e9d3bf
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,103,533	+0
android	hermes	armeabi-v7a	6,471,857	+0
android	hermes	x86	7,521,168	+0
android	hermes	x86_64	7,380,054	+0
android	jsc	arm64-v8a	8,968,359	+0
android	jsc	armeabi-v7a	7,699,467	+0
android	jsc	x86	9,030,658	+0
android	jsc	x86_64	9,508,690	+0

Base commit: 6e9d3bf
Branch: main

[pull-bot]

PR build artifact for d396973 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[ryancat]

Please take a look at the comments. Thanks!

[cipolleschi]

Hi @ZihanChen-MSFT, thank you so much for this addition.

I think we should slightly change the signature of the methods and try to keep the functions simpler not to hide side effects and additional computation in them.
I left some comments pointing out what I mean.

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ZihanChen-MSFT]

I don't know why I can only "Re-request review" for one reviewer, if I click another, the original one will be reset.

[pull-bot]

PR build artifact for fa1e1f7 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

I did some internal research on the issue @ZihanChen-MSFT and @christophpurrer.

Internally, we have modules that returns these type of promises:

• Promise<string[]>
• Promise<Object[]>
• Promise<number | null>
• Promise<empty>
• Promise<{ [string]: 'authorized' | 'denied' | 'undetermined' | true | false }>

For the Array, I think we have to support them as proper generic type inside a promise. It make sense to have a function in a module that returns a list of something.

The Promise<number | null> it sounds a lot like an optional. I am not a flow expert, but shouldn't it be Promise<number?>?

The Promise<empty> should be easy to handle. We can just support the EmptyTypeAnnotation. Although, I don't know what it means. Perhaps I can ask the team to replace it with void and see what happens.

Otherwise, the easy way for the last three is to threat the Promise of an unsupported type as Promise<any>, as much as we were doing before. This is probably the most conservative, backward compatible, approach.

What do you think?

[ZihanChen-MSFT]

@cipolleschi Hi, I've submitted one last commit and with a small fix it could support all types except number | null.

Fully supporting union as nullable will add a big piece of code, and I don't think I want to make so many changes into one PR. Fixing the same thing in TypeScript already introduced multiple PRs. Doing this in Flow could not be easier.

Do you think it is acceptable to skip the type if it is not recognized? The previous version didn't check them at all, so by doing this it doesn't make things worse.

By the way, number? means number | null | undefined, it is not number | null. If any internal person could change it to number? than we could try and see if we could remove the "skip if failed". I don't like to patch just to make "number | null" works.

[pull-bot]

PR build artifact for c933fa0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[pull-bot]

PR build artifact for 444d943 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[cipolleschi]

:( We have another type that did not pop up earlier: Promise<AuthorizationStatus>;, where AuthorizationStatus is:

export type AuthorizationStatus = 'authorized' | 'denied' | 'undetermined';

And the error is:

UnsupportedTypeAnnotationParserError: Module NativeTwilightMessenger: Flow type annotation 'StringLiteralTypeAnnotation' is unsupported in NativeModule specs.

Which is a bit weird, because it should have been catched by the latest changes, right?

[ZihanChen-MSFT]

Which is a bit weird, because it should have been catched by the latest changes, right?

But the type is actually supported, I've tested that in a new submitted test case.

[pull-bot]

PR build artifact for b7d90ec is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[cipolleschi]

My bad, the error was much subtler than what I understood at first sight.

So, the failing example is something like this:

export type CustomObject = {|
  field1: Array<Object>,
  field2: boolean,
  field3: string,
  type: 'A_String_Literal',
|};

export interface Spec extends TurboModule {
  // ...
  someMethod() => Promise<CustomObject>;
  // ...
}

And it fails when emitting the Promise<CustomObject> because it has a member that is type: 'A_String_Literal'.

[ZihanChen-MSFT]

@cipolleschi Hi, I add a new test case and fix the issue. I think 'a' should be treated like 'a' | 'b' | ... if it is a valid case, comparing to when we have only one member in an enum.

[github-actions]

	Warnings
⚠️	packages/react-native-codegen/src/parsers/typescript/modules/index.js#L57 - packages/react-native-codegen/src/parsers/typescript/modules/index.js line 57 – 'UnsupportedArrayElementTypeAnnotationParserError' is assigned a value but never used. (no-unused-vars)

Generated by 🚫 dangerJS against c3a65c0

[pull-bot]

PR build artifact for c3a65c0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[cipolleschi]

Thanks for the changes, they make sense to me. Also, many thanks for adding those tests: they should help us keeping an eye on those changes.
Let me import the PR and see if the internal CI is happy with the changes! :D

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @ZihanChen-MSFT in 8a38e03.

^{When will my fix make it into a release? | Upcoming Releases}