Add optional annotation in serialize step in codegen objective C output - #51636
Add optional annotation in serialize step in codegen objective C output#51636aladine wants to merge 11 commits into
Conversation
|
Hi @aladine! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
cipolleschi
left a comment
There was a problem hiding this comment.
@aladine thanks for working on this.
I read through the code and it makes sense to me.
|
@aladine can you fix the JS linter, please? |
2304dd8 to
fc1519c
Compare
|
@cipolleschi : I just updated lint and snapshot tests. |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this in D78265319. |
|
For the latest lint error https://github.com/facebook/react-native/actions/runs/15977428423/job/45918230480, I run prettier locally and it doesn't detect any lint problem. |
|
I run it locally. It applies this change in the }>) =>
- `${isOptional ? '@optional\n' : '@required\n'}- (${returnObjCType})${methodName}${params};`;
+ `${
+ isOptional ? '@optional\n' : '@required\n'
+ }- (${returnObjCType})${methodName}${params};`;
export type StructParameterRecord = $ReadOnly<{
|
|
Thanks Riccardo, I have fixed the lint based on your suggestion. |
|
how does this behave for Android? I know that the PR is only for iOS, but if we define an optional method, currently Android is forced to implement it, right? |
|
That is a good question. For Android, the abstract method has a default no-ops implementation in generated class If there is no implementation for optional method in Turbo class(which extends the generated spec class), it will fallback to the default no-ops block. |
|
this is a bit stuck internally. The vision here is to move to a subclass approach, but that would be a breaking change for the ecosystem. This needs more discussion, unfortunately. |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR is stale because it has been open for 180 days with no activity. It will be closed in 7 days unless you comment on it or remove the "Stale" label. |
Summary:
Added support for optional methods in Objective-C protocols. This change allows marking methods as optional in native modules by adding the optional property to method definitions. Optional methods are properly grouped under a single
@optionaldirective in the generated protocol, improving code organization and readability. Note that in Objective-C,@optionaldirective will apply for all the subsequent methods in protocol block so we have to move all optional methods at the end. This is implemented by inserting optional/required marker in ProtocolMethodTemplate in intermediate processing step.Changelog:
[IOS] [CHANGED] - Support optional methods in native module protocols
Test Plan:
Updated test case GenerateModuleHObjCpp-test.js.snap with optional methods to ensure they are properly marked with
@optionalin the generated protocol and grouped together in the output.