Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Codegen] feat: handle negative values in enums #47452

Conversation

okwasniewski
Copy link
Contributor

Summary:

This PR adds support for negative values in enums.

Currently when we try to use an enum with negative value:

enum MyEnum {
  ZERO = 0,
  POSITIVE = 1,
  NEGATIVE = -1,
}

export interface Spec extends TurboModule {
  useArg(arg: MyEnum): void;
}

export default TurboModuleRegistry.get<Spec>('Foo');

It will fail:

Enum values can not be mixed. They all must be either blank, number, or string values.

This is because negative values are parsed as UnaryExpressions which have - operator in front and value as argument.

With the new approach codegen properly generates enums with negative values.

Changelog:

[GENERAL] [ADDED] - Support negative values in enums

Test Plan:

I've added tests to see if everything is working properly

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 6, 2024
@okwasniewski okwasniewski force-pushed the feat/codegen-enums-handle-negative-values branch 2 times, most recently from 2c7d75d to 667fa99 Compare November 6, 2024 13:32
@okwasniewski okwasniewski force-pushed the feat/codegen-enums-handle-negative-values branch from 667fa99 to 12b7143 Compare November 6, 2024 13:40
Copy link
Member

@elicwhite elicwhite left a comment

Choose a reason for hiding this comment

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

Ah, yep, this is definitely something that needs to be supported.

You'll need to add support for this to Flow too.

Please add an example that exercises this code path to the parser fixture file here: https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/typescript/modules/__test_fixtures__/fixtures.js

You'll also need to modify the same fixture in the same way for Flow here: https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/flow/modules/__test_fixtures__/fixtures.js

the fixtures should stay in sync between Flow and TypeScript.

Also, since unary types aren't just negative, can you add a case to the failure fixture file in the same directory for something to exercise the failure case, like

enum Foo {
  BAD = !4
}

let enumMembersType: ?NativeModuleEnumMemberType = null;

switch (enumInitializerType) {
case undefined:
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a comment explaining this case, because it's very very strange to see undefined as a case in a switch. It's almost worth pulling it out into its own if statement above the switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, I've extracted this to a separate if statement above the switch

@okwasniewski
Copy link
Contributor Author

@elicwhite Thanks for your review! I applied your suggestions and added test cases for both Flow and Typescript.

Looks like Flow handles this case without any changes. Let me know if I should fix anything else

@okwasniewski okwasniewski force-pushed the feat/codegen-enums-handle-negative-values branch from d993309 to d38fffd Compare November 12, 2024 14:14
Copy link
Member

@elicwhite elicwhite left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!

I think this is the last thing before I will import.

@okwasniewski okwasniewski force-pushed the feat/codegen-enums-handle-negative-values branch from 91ed5e0 to 94c5486 Compare November 13, 2024 09:47
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 13, 2024
@facebook-github-bot
Copy link
Contributor

@elicwhite merged this pull request in 177bf4d.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @okwasniewski in 177bf4d

When will my fix make it into a release? | How to file a pick request?

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. Merged This PR has been merged. p: Callstack Partner: Callstack Partner 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.

4 participants