Skip to content

Add optional annotation in serialize step in codegen objective C output - #51636

Open
aladine wants to merge 11 commits into
react:mainfrom
aladine:codegen-optional-method
Open

Add optional annotation in serialize step in codegen objective C output#51636
aladine wants to merge 11 commits into
react:mainfrom
aladine:codegen-optional-method

Conversation

@aladine

@aladine aladine commented May 27, 2025

Copy link
Copy Markdown

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.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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 cla@meta.com. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 27, 2025
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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 facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 27, 2025

@Hardanish-Singh Hardanish-Singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cipolleschi cipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladine thanks for working on this.
I read through the code and it makes sense to me.

@cipolleschi

Copy link
Copy Markdown
Contributor

@aladine can you fix the JS linter, please?

@aladine
aladine force-pushed the codegen-optional-method branch from 2304dd8 to fc1519c Compare June 12, 2025 18:04
@aladine

aladine commented Jun 13, 2025

Copy link
Copy Markdown
Author

@cipolleschi : I just updated lint and snapshot tests.

@aladine
aladine requested a review from Hardanish-Singh June 30, 2025 15:51
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@aladine

aladine commented Jul 14, 2025

Copy link
Copy Markdown
Author

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.

@cipolleschi

Copy link
Copy Markdown
Contributor

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<{

@aladine

aladine commented Jul 15, 2025

Copy link
Copy Markdown
Author

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

@cipolleschi

Copy link
Copy Markdown
Contributor

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?

@aladine

aladine commented Jul 18, 2025

Copy link
Copy Markdown
Author

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.

@cipolleschi

Copy link
Copy Markdown
Contributor

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.

@react-native-bot

Copy link
Copy Markdown
Collaborator

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.

@react-native-bot react-native-bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Feb 9, 2026
@react-native-bot

Copy link
Copy Markdown
Collaborator

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.

@react-native-bot react-native-bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants