react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Add optional annotation in serialize step in codegen objective C output

Open aladine opened this issue 8 months ago • 2 comments

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 @optional directive in the generated protocol, improving code organization and readability. Note that in Objective-C, @optional directive 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 @optional in the generated protocol and grouped together in the output.

aladine avatar May 27 '25 19:05 aladine

Hi @aladine!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot avatar May 27 '25 19:05 facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot avatar May 27 '25 20:05 facebook-github-bot

@aladine can you fix the JS linter, please?

cipolleschi avatar Jun 12 '25 14:06 cipolleschi

@cipolleschi : I just updated lint and snapshot tests.

aladine avatar Jun 13 '25 21:06 aladine

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this in D78265319.

facebook-github-bot avatar Jul 14 '25 10:07 facebook-github-bot

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.

aladine avatar Jul 14 '25 14:07 aladine

I run it locally. It applies this change in the serializeMethod.js file:

}>) =>
-  `${isOptional ? '@optional\n' : '@required\n'}- (${returnObjCType})${methodName}${params};`;
+  `${
+    isOptional ? '@optional\n' : '@required\n'
+  }- (${returnObjCType})${methodName}${params};`;
export type StructParameterRecord = $ReadOnly<{

cipolleschi avatar Jul 14 '25 16:07 cipolleschi

Thanks Riccardo, I have fixed the lint based on your suggestion.

aladine avatar Jul 15 '25 18:07 aladine

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?

cipolleschi avatar Jul 18 '25 09:07 cipolleschi

That is a good question. For Android, the abstract method has a default no-ops implementation in generated class {} https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/generators/modules/GenerateModuleJavaSpec.js#L117

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.

aladine avatar Jul 18 '25 15:07 aladine

this is a bit stuck internally. This was the initial implementation for this feature, but then we decided not to use the @optional.

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.

cipolleschi avatar Aug 12 '25 16:08 cipolleschi