-
Notifications
You must be signed in to change notification settings - Fork 25k
feat: add getState for StateWrapperImpl
#48255
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
feat: add getState for StateWrapperImpl
#48255
Conversation
| jni::local_ref<ReadableNativeMap::jhybridobject> getStateDataImpl(); | ||
| void updateStateImpl(NativeMap* map); | ||
| void setState(std::shared_ptr<const State> state); | ||
| const std::shared_ptr<const State> getState() const; |
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.
The first const doesn't add any value
| const std::shared_ptr<const State> getState() const; | |
| std::shared_ptr<const State> getState() const; |
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.
On a second thought, I think returning a const ref might be a better option here to avoid an unnecessary copy?
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.
Yes, that's fine too. Although you do have to make sure you don't hold on to the shared_ptr beyond the lifecycle of the StateWrapperImpl then.
…tateWrapperImpl.h Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>
|
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| jni::local_ref<ReadableNativeMap::jhybridobject> getStateDataImpl(); | ||
| void updateStateImpl(NativeMap* map); | ||
| void setState(std::shared_ptr<const State> state); | ||
| const std::shared_ptr<const State>& getState() const; |
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.
The state returned here is mutable, so returning a reference here seems a bit dangerous. We're making some assumptions about the fact that the caller will manage the lifecycle correctly. Maybe it'd be better to just return a copy of the shared pointer.
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.
alright, agree - let me change!
|
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @hannojg in ed36e89 When will my fix make it into a release? | How to file a pick request? |
Summary:
We're trying to pass
jsi::Values directly to our view components (and convert them to java/swift types manually). That way we can pass "complex" objects to our views (such asjsi::Objects withNativeStateattached, without the need to convert them to e.g.folly::dynamic).On android we store our complex prop values on the
StateWrapperImplto pass the complex types between c++ and java/kotlin. See an example here:https://github.com/hannojg/nitro/blob/2378fe7754294c496b2cbcd62f7109529e276427/packages/react-native-nitro-image/nitrogen/generated/android/c%2B%2B/JValueFromStateWrapper.cpp#L21-L23
For that we need to be able to access the underlying state, which is what we added in this PR.
Changelog:
[ANDROID] [ADDED] - Added
getStatemethod forStateWrapperImplTest Plan:
Internal change, just make sure all tests are passing