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
Android - pass initial props to ViewManager createView in non-Fabric #31053
Conversation
Hi @gkode! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java
Show resolved
Hide resolved
|
Base commit: eeb36f4 |
Base commit: eeb36f4 |
@gkode Can you specify defaults in your custom view manager that will be used if null props are passed in? That seems like an easier change and wouldn't require upstreaming anything. For this sort of change we would need a more comprehensive test plan than "tested manually" - how does this impact components in the ecosystem in general? Could it break anything? A few lines below your change, |
In the future I would also recommend giving PRs a more descriptive title to help maintainers. I updated this one for you :) Other thoughts:
I think I'd be okay making this change in the spirit of consistency between Fabric and non-Fabric, but I would want to see a more reproducible and fleshed-out test plan first. |
While createViewInstance, I want to provide already existing view in memory on the basis of provided props to Native hierarchy. But in Update properties, view is already created and added in native view hierarchy. So, I will have to replace my existing view (in memory) in update properties method with the one created in CreateViewInstance which makes my code a bit messy. Can you please help with making test plan for it ? What all test we want to pass on it to make sure it doesn't break anything. I tested this change on android only. |
I realized that some of our FB-internal view managers use the presence of props in the constructor to differentiate between Fabric and non-Fabric rendering modes. I will need to think about this a bit more. |
is it really right parameter to differentiate between fabric and non-fabric? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@JoshuaGross What's the plan to take this change forward |
@JoshuaGross Any update on it. |
@JoshuaGross merged this pull request in ea34953. |
Merged @gkode, just took a while to audit everything relying on this internally |
Summary
#31051
When trying to create custom viewmanagers, we don't have props, I have a use case where I want to create views on the basis of provided react prop from react native.
Changelog
[CATEGORY] [TYPE] - ReactNative WebView
Test Plan
Tested Manually.