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

Add missing C++ include for prop conversion of complex array type #35984

Closed
wants to merge 1 commit into from

Conversation

rshest
Copy link
Contributor

@rshest rshest commented Jan 26, 2023

Summary:
[Changelog][Internal]

Codegen for props parsing was failing to add a required include for the case when the type is an array of objects, which in turn use non-trivial types.

Something like:

export type NativeProps = $ReadOnly<{
  ...ViewProps,
  bounds: $ReadOnlyArray<
    $ReadOnly<{
      height?: Float,
      left?: Float,
      top?: Float,
      width?: Float,
    }>,
  >,
}>;

would cause compilation errors on C++ side, since the required header for the Float conversion wasn't included.

Reviewed By: cipolleschi

Differential Revision: D42781128

Summary:
[Changelog][Internal]

Codegen for props parsing was failing to add a required include for the case when the type is an array of objects, which in turn use non-trivial types.

Something like:
```
export type NativeProps = $ReadOnly<{
  ...ViewProps,
  bounds: $ReadOnlyArray<
    $ReadOnly<{
      height?: Float,
      left?: Float,
      top?: Float,
      width?: Float,
    }>,
  >,
}>;
```

would cause compilation errors on C++ side, since the required header for the `Float` conversion wasn't included.

Reviewed By: cipolleschi

Differential Revision: D42781128

fbshipit-source-id: 5b0ad5c3f5740c1bcb308e56cc4c1854c8f71a6a
@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: Facebook Partner: Facebook Partner fb-exported labels Jan 26, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42781128

@github-actions
Copy link

Fails
🚫

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 0d30513

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,463,042 +0
android hermes armeabi-v7a 7,783,401 +0
android hermes x86 8,936,022 +0
android hermes x86_64 8,793,929 +0
android jsc arm64-v8a 9,648,914 +0
android jsc armeabi-v7a 8,383,184 +0
android jsc x86 9,710,884 +0
android jsc x86_64 10,187,735 +0

Base commit: 53932d0
Branch: main

@github-actions
Copy link

This pull request was successfully merged by @rshest in a00cea4.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Jan 27, 2023
cipolleschi pushed a commit that referenced this pull request Feb 13, 2023
…5984)

Summary:
Pull Request resolved: #35984

[Changelog][Internal]

Codegen for props parsing was failing to add a required include for the case when the type is an array of objects, which in turn use non-trivial types.

Something like:
```
export type NativeProps = $ReadOnly<{
  ...ViewProps,
  bounds: $ReadOnlyArray<
    $ReadOnly<{
      height?: Float,
      left?: Float,
      top?: Float,
      width?: Float,
    }>,
  >,
}>;
```

would cause compilation errors on C++ side, since the required header for the `Float` conversion wasn't included.

Reviewed By: cipolleschi

Differential Revision: D42781128

fbshipit-source-id: d5b133b931a60e414761db0b3ed09893d3fcc9aa
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…cebook#35984)

Summary:
Pull Request resolved: facebook#35984

[Changelog][Internal]

Codegen for props parsing was failing to add a required include for the case when the type is an array of objects, which in turn use non-trivial types.

Something like:
```
export type NativeProps = $ReadOnly<{
  ...ViewProps,
  bounds: $ReadOnlyArray<
    $ReadOnly<{
      height?: Float,
      left?: Float,
      top?: Float,
      width?: Float,
    }>,
  >,
}>;
```

would cause compilation errors on C++ side, since the required header for the `Float` conversion wasn't included.

Reviewed By: cipolleschi

Differential Revision: D42781128

fbshipit-source-id: d5b133b931a60e414761db0b3ed09893d3fcc9aa
@cipolleschi cipolleschi mentioned this pull request Oct 11, 2023
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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants