feat(ReactNative): prioritize attribute config process function to allow processing function props
#32119
Conversation
hannojg
changed the title
process function to allow processing function props
|
This is an interesting direction, and likely we can support this without major changes. It does mean that these function props completely bypass React's event dispatching model, which I think warrants further discussion. Maybe all direct events should be passed through like this? Or maybe we can should change the event emitter API so you can emit direct callbacks as if they were a function (with a return value)? In the short-term, please add unit tests here to demonstrate the changed behaviour. |
| } else if (typeof attributeConfig.process === 'function') { | ||
| // An atomic prop with custom processing. | ||
| newValue = attributeConfig.process(prop); | ||
| } else if (typeof prop === 'function') { | ||
| // A function prop. It represents an event handler. Pass it to native as 'true'. | ||
| newValue = true; | ||
| } else if (typeof attributeConfig !== 'object') { | ||
| // An atomic prop. Doesn't need to be flattened. | ||
| newValue = prop; | ||
| } else if (typeof attributeConfig.process === 'function') { | ||
| // An atomic prop with custom processing. | ||
| newValue = attributeConfig.process(prop); | ||
| } else if (typeof attributeConfig.diff === 'function') { | ||
| // An atomic prop with custom diffing. We don't need to do diffing when adding props. |
There was a problem hiding this comment.
I'm not sure this is correct. You're assuming now that attributeConfig is always going to be an object which you can read process function of.
There was a problem hiding this comment.
Right, good catch tanks. Added an additional check for object. Will also add unit tests in a moment
hannojg
force-pushed
the
feat/prioritize-view-attribute-process-function
branch
from
a2cc692 to
7adf510
Compare
This would probably require quite a few changes on the native side. For reference, this is how Nitro converts a But I can definitely look into this as a future improvement for react-native core/TurboModules once this PR is merged - this is one of the places where Nitro has safer memory management and better performance as well - so maybe I can upstream this somehow :) |
Could you please share more details about the motivation for this change? What's your use case? |
We have a custom framework (Nitro) that allows you to build native modules and soon also native components for React Native. Instead of going through React Native's default prop converter (JS value ( We built this, and it works for every prop so far (e.g. We dug around the code and found out that this is because RN handles functions/callbacks differently - they are not retained on native, but instead built as a event dispatcher system on the JS side, loosely called from native. This PR allows to change this behaviour if needed - so no breaking change for existing code, but with a custom view config we can adjust it for our cases. |
|
Added the unit test @javache . One question though, I opened another PR in react native for updating the Is it the right place to do it in /react-native or should it be added to /react (like this change) ? |
Following up with @rubennorte on this - seems like this fork is only used for |
|
You've only changed |
|
Also, could this work as a workaround? Instead of passing the function through directly, wrap it in another object. On the native side, it just means you have an extra jsi::Object to unwrap. |
…we have a process function
hannojg
force-pushed
the
feat/prioritize-view-attribute-process-function
branch
from
11eb8f8 to
9e74020
Compare
|
Good again for review (thanks for taking the time and effort!) |
|
Hey, while we know that there is a workaround by wrapping the function in an object, we are still very eager to get this one fixed. |
|
Hey - bumping this, is there any chance we can get this reviewed again? @javache 🙏 |
|
Sorry, this looks ready to merge! |
…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)
|
lfg! thanks so much |
The prettier check for this file is currently failing on `main`, after facebook#32119 was merged.
The prettier check for this file is currently failing on `main`, after #32119 was merged.
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
ViewConfigattributes already have the API for aprocessfunction. 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:
How did you test this change?
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:
compressed.mp4
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: trueormyProp: { 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: truefor callback props, so this change should be fine.