Skip to content

feat: use process function if prop is a function but view attribute configured a process function #48777

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: use process function if prop is a function but view attribute configured a process function #48777

wants to merge 1 commit into from

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented Jan 18, 2025 •
edited
Loading

Summary:

Motivation

As per the ViewConfig flow types we can configure a custom process function for props:

react-native/packages/react-native/Libraries/Renderer/shims/ReactNativeTypes.js

Lines 38 to 43 in 22e7691

export type AttributeType<T, V> =
| true
| $ReadOnly<{
diff?: (arg1: T, arg2: T) => boolean,
process?: (arg1: V) => T,
}>;

However, this process function is ignored in the diffProperties function when the incoming prop is a function.

This is a problem, as we have no mechanism to opt-out of the default behaviour of a function prop being set to true. In our case we are trying to pass the props as direct JSI values to our View components, but right now function props are always received as true, due to this forced behaviour and we want to opt out.

This PR implements a check to call the process function if it's set. This way we can opt-out of passing functions as true to the native side instead of the actual function.

Note: this is only one piece of the puzzle, as this flow only gets triggered for updating props on a component (as far as I can see). The flow for when a component gets created is different and is handled here:

Changelog:

[GENERAL] [ADDED] - Call process for a prop's view config, even if that prop is a function.

Test Plan:

I modified the code manually in a template react-native app and confirmed its working. This is a code path you only need in very special cases, thus it's a bit hard to provide a test for this. I recorded a video where you can see that the changes are active and the prop is being passed as native value once the props for a component get updated:

compressed2.mp4

@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 Jan 18, 2025
@hannojg
Copy link
Contributor Author

hannojg commented Jan 18, 2025

Test and lint are changing for things I haven't touched 🤔

@hannojg hannojg mentioned this pull request Jan 18, 2025
Merged
@hannojg hannojg marked this pull request as ready for review January 18, 2025 16:56
@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 Jan 18, 2025
@mrousavy mrousavy mentioned this pull request Jan 21, 2025
Closed
@javache
Copy link
Member

javache commented Jan 30, 2025

This code has gotten out-of-sync with what's upstream. Let's land this change in React and I'll look into merging it here.

It may take a while before you'll see this fix though (and the approach of wrapping the function in an object may be required) since we'd need to wait for a next React release.

@javache javache closed this Jan 30, 2025
javache pushed a commit to facebook/react that referenced this pull request Jun 9, 2025
@hannojg
feat(ReactNative): prioritize attribute config process function to …
911dbd9 
…allow processing function props (#32119)

## Summary

In react-native props that are passed as function get converted to a
boolean (`true`). This is the default pattern for event handlers in
react-native.
However, there are reasons for why you might want to opt-out of this
behavior, and instead, pass along the actual function as the prop.
Right now, there is no way to do this, and props that are functions
always get set to `true`.
The `ViewConfig` attributes already have the API for a `process`
function. I simply moved the check for the process function up, so if a
ViewConfig's prop attribute configured a process function this is always
called first.
This provides an API to opt out of the default behavior. 

This is the accompanied PR for react-native:

- facebook/react-native#48777

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

I modified the code manually in a template react-native app and
confirmed its working. This is a code path you only need in very special
cases, thus it's a bit hard to provide a test for this. I recorded a
video where you can see that the changes are active and the prop is
being passed as native value.

For this I created a custom native component with a view config that
looked like this:

```js
const viewConfig = {
  uiViewClassName: 'CustomView',
  bubblingEventTypes: {},
  directEventTypes: {},
  validAttributes: {
    nativeProp: {
      process: (nativeProp) => {
		// Identity function that simply returns the prop function callback
        // to opt out of this prop being set to `true` as its a function
        return nativeProp
      },
    },
  },
}
```



https://github.com/user-attachments/assets/493534b2-a508-4142-a760-0b1b24419e19

Additionally I made sure that this doesn't conflict with any existing
view configs in react native. In general, this shouldn't be a breaking
change, as for existing view configs it didn't made a difference if you
simply set `myProp: true` or `myProp: { process: () => {...} }` because
as soon as it was detected that the prop is a function the config
wouldn't be used (which is what this PR fixes).
Probably everyone, including the react-native core components use
`myProp: true` for callback props, so this change should be fine.
github-actions bot pushed a commit to facebook/react that referenced this pull request Jun 9, 2025
@javache
feat(ReactNative): prioritize attribute config process function to …
a93b8db 
…allow processing function props (#32119)

## Summary

In react-native props that are passed as function get converted to a
boolean (`true`). This is the default pattern for event handlers in
react-native.
However, there are reasons for why you might want to opt-out of this
behavior, and instead, pass along the actual function as the prop.
Right now, there is no way to do this, and props that are functions
always get set to `true`.
The `ViewConfig` attributes already have the API for a `process`
function. I simply moved the check for the process function up, so if a
ViewConfig's prop attribute configured a process function this is always
called first.
This provides an API to opt out of the default behavior.

This is the accompanied PR for react-native:

- facebook/react-native#48777

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

I modified the code manually in a template react-native app and
confirmed its working. This is a code path you only need in very special
cases, thus it's a bit hard to provide a test for this. I recorded a
video where you can see that the changes are active and the prop is
being passed as native value.

For this I created a custom native component with a view config that
looked like this:

```js
const viewConfig = {
  uiViewClassName: 'CustomView',
  bubblingEventTypes: {},
  directEventTypes: {},
  validAttributes: {
    nativeProp: {
      process: (nativeProp) => {
		// Identity function that simply returns the prop function callback
        // to opt out of this prop being set to `true` as its a function
        return nativeProp
      },
    },
  },
}
```

https://github.com/user-attachments/assets/493534b2-a508-4142-a760-0b1b24419e19

Additionally I made sure that this doesn't conflict with any existing
view configs in react native. In general, this shouldn't be a breaking
change, as for existing view configs it didn't made a difference if you
simply set `myProp: true` or `myProp: { process: () => {...} }` because
as soon as it was detected that the prop is a function the config
wouldn't be used (which is what this PR fixes).
Probably everyone, including the react-native core components use
`myProp: true` for callback props, so this change should be fine.

DiffTrain build for [911dbd9](911dbd9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hannojg @javache @facebook-github-bot