Skip to content

PoC: use raw jsi::Value prop w/ fabric views#386

Closed
hannojg wants to merge 20 commits into
mrousavy:mainfrom
hannojg:poc/views-with-nativestate-props
Closed

PoC: use raw jsi::Value prop w/ fabric views#386
hannojg wants to merge 20 commits into
mrousavy:mainfrom
hannojg:poc/views-with-nativestate-props

Conversation

@hannojg
Copy link
Copy Markdown
Contributor

@hannojg hannojg commented Dec 3, 2024

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nitro-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 1:44pm

Comment thread example/package.json
"lint-ci": "eslint \"**/*.{js,ts,tsx}\" -f @jamesacarr/github-actions",
"bundle-install": "bundle install",
"pods": "cd ios && bundle exec pod install && rm -rf .xcode.env.local",
"pods": "cd ios && bundle exec pod install",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: without this change i can't run the example app. I am pretty sure that deleting the .xcode.env.local is wrong.
Will remove this change in a moment though

@hannojg
Copy link
Copy Markdown
Contributor Author

hannojg commented Dec 3, 2024

Okay, i am still missing one piece, seems like the props parser currently removes our prop:

[RawPropsParser.cpp:75] Looked up property name which was not seen when preparing: nativeProp

fixing …

Comment thread packages/react-native-nitro-image/android/CMakeLists.txt Outdated
prop updates are broken yet
Moved code to set props to state to adopt() as thats always called for creation & updates
@hannojg
Copy link
Copy Markdown
Contributor Author

hannojg commented Dec 13, 2024

@mrousavy android is good as well now.

In the same phase in which the new props are updated we get the update on the java side, so I think there is no weird back and forth.

We have to use the state wrapper on android as this is the only thing thats shared between c++ and java for the view manager (props are always eagerly converted to folly::dynamic).

@hannojg
Copy link
Copy Markdown
Contributor Author

hannojg commented Dec 13, 2024

Wait, updating the patch file …

Updated 👍

@hannojg hannojg mentioned this pull request Dec 16, 2024
5 tasks
@hannojg hannojg changed the title [WIP] poc: use raw jsi::Value prop w/ fabric views PoC: use raw jsi::Value prop w/ fabric views Dec 16, 2024
}

export const CustomView: HostComponent<NativeProps> =
NativeComponentRegistry.get<NativeProps>('CustomView', () => viewConfig)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does requireNativeComponent(..) not work here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might, we'd have to test this

Comment on lines +32 to +36
// There is some code path in RawPropsParser.cpp where we expect to collect all possible keys (prop names) upfront
// during the init phase. Calling this .at() method while the rawProps is empty, will cause this prop value to be indexed …
// Otherwise we'd get a crash when trying to access it later.
// We should really also try to merge the PR i have for providing alternative RawProps parser !!
rawProps.at("nativeProp", nullptr, nullptr);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I fully understand - is this still needed? Do I need to do this for every prop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just calling rawProps.at() and check if it is null or not should be enough. It should never be null, except for when the internally the ui manager creates those once without the values from JS (thats when 'isEmpty() will return true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but yeah, it has to be called at this point once, as the .at() method will insert the props key into a map. If later on the prop name is not in this key map it will always return null. Super weird I know.
my main idea is that we are able to write our own prop parser to work around this

Comment on lines +73 to +88
# From node_modules/react-native/ReactAndroid/cmake-utils/folly-flags.cmake
# Used in node_modules/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake
target_compile_definitions(
NitroImage
PRIVATE
-DFOLLY_NO_CONFIG=1
-DFOLLY_HAVE_CLOCK_GETTIME=1
-DFOLLY_USE_LIBCPP=1
-DFOLLY_CFG_NO_COROUTINES=1
-DFOLLY_MOBILE=1
-DFOLLY_HAVE_RECVMMSG=1
-DFOLLY_HAVE_PTHREAD=1
# Once we target android-23 above, we can comment
# the following line. NDK uses GNU style stderror_r() after API 23.
-DFOLLY_HAVE_XSI_STRERROR_R=1
)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I really need those..? @hannojg

Comment on lines +79 to +89
class ComponentShadowNode : public ConcreteViewShadowNode<CustomViewComponentName, CustomViewProps,
ViewEventEmitter, // default one
CustomStateData> {

public:
ComponentShadowNode(const ShadowNodeFragment& fragment, const ShadowNodeFamily::Shared& family, ShadowNodeTraits traits)
: ConcreteViewShadowNode(fragment, family, traits) {}

ComponentShadowNode(const ShadowNode& sourceShadowNode, const ShadowNodeFragment& fragment)
: ConcreteViewShadowNode(sourceShadowNode, fragment) {}
};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need this for android, or will the iOS code from above (using ... = ...) work instead of subclassing it here?

@mrousavy
Copy link
Copy Markdown
Owner

Now implemented in #512 🫡

@mrousavy mrousavy closed this Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants