Update example app to React Native 0.70 RC #408
Conversation
* Bring back Pods to cache * Use new-arch string in key for new arch cache * Try to reinstall pods instead of creating new * Separate deps installation between two caches * Rename Pods reinstall step
| const std::string name, | ||
| const std::shared_ptr<CallInvoker> jsInvoker) override; | ||
| const std::string &name, | ||
| const std::shared_ptr<CallInvoker> &jsInvoker) override; |
There was a problem hiding this comment.
Why passing shared pointer as a reference? This removes the idea of a pointer being actually shared.
Making a copy allows this smart pointer to do it's job of sharing the pointer.
There was a problem hiding this comment.
It's a part of upgrade process for RN 0.70: https://react-native-community.github.io/upgrade-helper/?from=0.69.1&to=0.70.0-rc.1
There was a problem hiding this comment.
Well, the idea of how shared pointer works is that each time new copy is created, the internal counter of copies of this shared pointer is increased, so the smart pointer can manage the allocated space for example: when to free the memory. If we pass the shared pointer as a reference, we are not increasing it and work on a direct object of a copy that was passed to that method. So why having it a shared pointer in the first place then?
Consider the above a hint, please, but I'm eager to "check" if the example app will work with a new architecture with having it still passed by a copy, though.
I see it's a part of official migration guidence, Don't get me wrong, my point is: if there's any reason behind that line in original guide other than "you should use const reference wherever possible" then I'm happy to know and learn.
okwasniewski
changed the title
Sorry for not providing the PR description right away. I'm testing the react native autolinking for new arch on the example app. The main blocker is the release of React Native Windows 0.70 RC once it's released then we're good to go with the upgrade. This PR is still a draft, that's why there is newArch set as default I will turn it back to off once this PR is ready for review. Anyway we can still ship the new architecture RC of slider without this example being upgraded. |
okwasniewski
force-pushed
the
chore/fabric-android
branch
2 times, most recently
from
b734a21 to
3701d57
Compare
Summary:
Update example app to RN 0.70 to allow for autolinking on android.
Currently the main blocker is the release of RNW 0.70 RC.
Test Plan: