Handle (T) and undefined properly in turbo module component codegen #34693
Conversation
facebook-github-bot
added
the
CLA Signed
ZihanChen-MSFT
force-pushed
the
ts-rncodegen8
branch
from
c34e1c2
to
fd3e88f
Compare
facebook-github-bot
added
the
Shared with Meta
There was a problem hiding this comment.
Thank you for taking the time for this PR.
I left a few comments and some improvements that we can apply. Let me know what do you think.
I also have the feeling that this PR is doing a little bit too much: support parenthesized types, changing null for undefined, refactoring, error handling... It will be better to split it in multiple PR, what do you think?
| string_optional_key?: (string); | ||
| string_optional_value: (string) | null | undefined; | ||
| string_optional_both?: (string | null | undefined); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -764,23 +764,23 @@ export interface ModuleProps extends ViewProps { | |||
| }> | |||
| >; | |||
|
|
|||
| onDirectEventDefinedInlineOptionalKey?: DirectEventHandler< | |||
| onDirectEventDefinedInlineOptionalKey?: (DirectEventHandler< | |||
There was a problem hiding this comment.
BubblingEventHandler cases below still covered syntax without parenthesis.
packages/react-native-codegen/src/parsers/typescript/components/__test_fixtures__/fixtures.js
Show resolved
Hide resolved
packages/react-native-codegen/src/parsers/typescript/components/events.js
Show resolved
Hide resolved
packages/react-native-codegen/src/parsers/typescript/components/props.js
Show resolved
Hide resolved
ZihanChen-MSFT
force-pushed
the
ts-rncodegen8
branch
from
f90d355
to
fa8137d
Compare
Base commit: ab5f26b |
Hi, @cipolleschi , in general I do think smaller PR is better, but in this case, the "first event type argument could be void->undefined or null" one is just one line of code and it is embedded in the refactored code, and if I split into 2 PR it will get conflict no matter which one is merged first. So in this particular case I do think combining them in one is better. |
Base commit: ab5f26b |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @ZihanChen-MSFT in 205cc9b. When will my fix make it into a release? | Upcoming Releases |
…acebook#34693) Summary: In `buildEventSchema` and `buildPropSchema`, they check into property types to see if the given property could be converted into an event schema or a property schema. The original implementation only handles limited cases, I refactor them and make them easier to maintain. In `getPropertyType` in `events.js`, it handles `(T)` at a wrong place, fixed. In `getPropertyType` in `props.js`, it doesn't handle `(T)`, fixed. And I also fixed some other issues to make the codegen reports error better. There are many duplicated test cases that cover every piece of the code, I changed some of them so that it tests both original cases and new cases. ## Changelog [General] [Changed] - Handle (T) and undefined properly in turbo module component codegen Pull Request resolved: facebook#34693 Test Plan: `yarn jest` passed in `packages/react-native-codegen` Reviewed By: NickGerleman Differential Revision: D39647075 Pulled By: cipolleschi fbshipit-source-id: 8e1df2b54aab37b7151d0bf74260e2eba0602777
Summary
In
buildEventSchemaandbuildPropSchema, they check into property types to see if the given property could be converted into an event schema or a property schema. The original implementation only handles limited cases, I refactor them and make them easier to maintain.In
getPropertyTypeinevents.js, it handles(T)at a wrong place, fixed.In
getPropertyTypeinprops.js, it doesn't handle(T), fixed.And I also fixed some other issues to make the codegen reports error better.
There are many duplicated test cases that cover every piece of the code, I changed some of them so that it tests both original cases and new cases.
Changelog
[General] [Changed] - Handle (T) and undefined properly in turbo module component codegen
Test Plan
yarn jestpassed inpackages/react-native-codegen