Skip to content

feat(android): allow passing filter function for prop folly::dynamic conversion#48202

Closed
hannojg wants to merge 4 commits into
facebook:mainfrom
hannojg:feat/allow-passing-filter-function-to-props-convertion-android
Closed

feat(android): allow passing filter function for prop folly::dynamic conversion#48202
hannojg wants to merge 4 commits into
facebook:mainfrom
hannojg:feat/allow-passing-filter-function-to-props-convertion-android

Conversation

@hannojg
Copy link
Copy Markdown
Contributor

@hannojg hannojg commented Dec 10, 2024

Summary:

Motivation

  • We need to exclude certain prop keys from conversion to folly::dynamic on android for our custom use case where we pass down jsi::objects with NativeState attached down the props

Otherwise we run into crashes such as:

CleanShot 2024-12-11 at 09 18 26@2x

Changes

  • dynamicFromValue was marked as noexcept although it can throw, I removed the noexcept for correctness
  • Made it so you can pass down a filter function to exclude certain props from conversion (using the existing mechanism for that)
    • I think there is no way to pass a filter function and retain it in RawProps as that is constructed very early on in UIManagerBinding

Changelog:

[INTERNAL] [ADDED] - Allow passing a filter function to BaseViewProps to exclude certain props on android from being dynamically casted

Test Plan:

You can try modifying for example ScrollViewProps.cpp and pass a fourth argument to ViewProps to confirm that the filtering is working:

ScrollViewProps::ScrollViewProps(
    const PropsParserContext& context,
    const ScrollViewProps& sourceProps,
    const RawProps& rawProps)
    : ViewProps(context, sourceProps, rawProps, [&](const std::string& keyName){
        return true;
    }),

@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 Dec 10, 2024
@hannojg hannojg force-pushed the feat/allow-passing-filter-function-to-props-convertion-android branch from b1ce84e to 7c6aac4 Compare December 11, 2024 08:38
@hannojg hannojg force-pushed the feat/allow-passing-filter-function-to-props-convertion-android branch from 7c6aac4 to ddd770a Compare December 11, 2024 08:48
@hannojg hannojg changed the title [WIP] feat(android): allow passing filter function for prop folly::dynamic conversion feat(android): allow passing filter function for prop folly::dynamic conversion Dec 11, 2024
@hannojg hannojg marked this pull request as ready for review December 11, 2024 09:05
@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 Dec 11, 2024
Comment thread packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp Outdated
Comment thread packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp Outdated
Comment thread packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp Outdated
hannojg and others added 2 commits December 11, 2024 12:04
….cpp

Co-authored-by: Marc Rousavy <marcrousavy@hotmail.com>
@facebook-github-bot
Copy link
Copy Markdown
Contributor

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

@hannojg hannojg requested a review from javache December 11, 2024 11:56
Comment thread packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp Outdated
….cpp

Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>
@hannojg hannojg requested a review from javache December 11, 2024 18:00
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@javache 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 Dec 12, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@javache merged this pull request in feff4a7.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @hannojg in feff4a7

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

5 participants