chore(Android): add autolinking on Fabric #1585
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For now it is only partially filtered (there is also code for other
components for now). I'm not sure whether I should leave all the
codegened code or leave only the RNSScreen part.
As we provide custom implementation
Setting `componentDescriptors` causes RNCLI to not search for autogenerated ones -> we need to specify all of them manually
This CMake builds both custom cpp code & generated one. It seems to me, that it must be done this way as project name recognized by autogenerated RN CLI cmake is `react_codegen_rnscreens` and we can not change that => only this library is included in final build.
IMPORTANT: this causes our custom `rnscreens.h` header to take precedence over its codegened counterpart when imported in following way in `rncli.cpp`: ```c++ ``` This is important beacuse it allows us to inject `RNSScreenComponentDescriptor` symbol into `rncli.cpp` translation unit scope.
We no longer build this target as we want to rely on new Android autolinking.
libreact_codegen_rnscreens.so library This symbol for some, unknown yet reason, is not visible during linking of lib<project-name>.so (exactly during linking rncli.cpp.o file). We enforce desired visibility with compiler attributte. Note however that this solution should be only temporary as the exact reason should be tracked down and the issue shall be fixed with more generic approach.
This file is not only no longer used, but also it does not contain valid configuration for our library. This file was replaced with common/cpp/CMakeLists.txt as we migrated to CMake build system.
Up to this point, some paths were hardcoded, this is no longer the case.
This is temporary workaround as `rnscreens.{h,cpp}` files are not part
of common cpp code - they are android specific I believe. They must be
moved to android JNI directory along with CMakeLists.txt
specific directory These files are not shared between iOS & Android platforms.
WoLewicki
reviewed
Sep 16, 2022
android/src/main/jni/CMakeLists.txt
Outdated
| set(RNS_COMMON_DIR ${RNS_ANDROID_DIR}/../common/cpp) | ||
| set(RNS_GENERATED_DIR ${RNS_ANDROID_DIR}/build/generated) | ||
| set(RNS_GENERATED_JNI_DIR ${RNS_GENERATED_DIR}/source/codegen/jni) | ||
| set(RNS_GENERATED_REACT_DIR ${RNS_GENERATED_JNI_DIR}/react/renderer/components/rnscreens) |
There was a problem hiding this comment.
Would it be possible to extract rnscreens to a variable so that library authors could change only that variable for their usecase?
There was a problem hiding this comment.
I guess I could do it, but it comes down to simply using find & replace option in code editor. I will, however refine variable names and do some cleanup.
There was a problem hiding this comment.
So I went with your suggestion. All it takes now is to change LIB_LITERAL constant. However some further customisation may be required as you noticed in RNSVG (adding custom dependencies etc.)
tomekzaw
reviewed
Sep 16, 2022
common/cpp/react/renderer/components/rnscreens/RNSScreenComponentDescriptor.h
Outdated
Show resolved
Hide resolved
explicitly This is better as it also handles Windows C++ compilers.
kkafar
force-pushed
the
@kkafar/fabric-autolinking
branch
from
b241138
to
3afe846
Compare
WoLewicki
approved these changes
Sep 19, 2022
| react_render_mapbuffer | ||
| rrc_view | ||
| turbomodulejsijni | ||
| yoga |
There was a problem hiding this comment.
Based on the changes needed in rnsvg, maybe it would be good to add a comment that you should add here all libraries that your custom state depend on.
| namespace facebook { | ||
| namespace react { | ||
|
|
||
| JSI_EXPORT |
There was a problem hiding this comment.
It would be nice to add information why it is needed at all.
1 task
kkafar
added a commit
that referenced
this pull request
Sep 19, 2022
## Description Update supported React Native version for Fabric in library's main readme. Support for older versions has been dropped as we added autolinking in #1585 (it [requires `@react-native-community/cli` 9.1.0 or newer](https://github.com/react-native-community/cli/releases/tag/v9.1.0) (support for `cmakeListsPath` in config) which is shipped with React Native since 0.70.0) ## Changes Since 3.18.0 Fabric will be supported only with React Native 0.70.0 or newer. ## Test code and steps to reproduce -- ## Checklist - [ ] Ensured that CI passes
1 task
kkafar
added a commit
that referenced
this pull request
Sep 19, 2022
## Description Add code comments for few changes introduced in #1585. ## Changes Just few code comments. See the changes. ## Test code and steps to reproduce -- ## Checklist - [x] Ensured that CI passes
1 task
kkafar
added a commit
that referenced
this pull request
Sep 23, 2022
## Description The `RNScreensComponentsRegistry` class was used for custom linking (loading of `rnscreen_modules` shared library to java runtime). With merge of #1585 it became redundant. ## Changes Removed unused class & its source file. ## Test code and steps to reproduce Just build the application and see that everything works as usual. ## Checklist - [ ] Ensured that CI passes
This was referenced Oct 11, 2022
kkafar
added a commit
that referenced
this pull request
Oct 11, 2022
## Description Autolinking was added with #1585 and it added `react-native.config.js` file with codegen configuration, **but** it did not add this file to npm include list causing its absence in final released package... Fixes #1608 ## Changes Added `react-native.config.js` to npm file list in main `package.json` ## Test code and steps to reproduce See #1608 for reproductions. ## Checklist - [x] Ensured that CI passes
This was referenced Oct 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR integrates the library with Android autolinking mechanism 🥳
Up to this moment,
react-native-screenshad custom linking mechanism which buildedrnscreens_modules&rnscreens_commonshared libraries and loaded them during runtime, leveraging JNI'sOnLoadmethod to add our custom component descriptors providers toComponentDescriptorProviderRegistryinstance. Since this PR forward, this custom loading mechanism is no longer needed as it shall be done by RNCLI autolinking.See my comments below this PR description to see comments on fragile changes.
IMPORTANT: This PR requires
@react-native-community/cli9.1.0or newer (support forcmakeListsPathinreact-native.config.js& Android autolinking). This version is shipped withreact-nativesince0.70.0=> with merging of this PR we drop support for olderreact-nativeversions on Fabric.See #1595 for additional commentary on important changes & implementation details.
I also recommend to go through discussion under this description to gain better understanding of changes.
Changes
rnscreen_modulesshared library.rnscreens_modulesSO.react_codegen_rnscreenstarget (replacement ofrnscreen_modules). See (1) below.(1)
Autolinking expects
react_codegen_rnscreensto be the name of the library. The name is autogenerated.I believe it would be possible to create separate target containing only custom cpp code with adding a line like:
in our custom
CMakeLists.txtsoreact_cdegen_rnscreenscontains only codegened symbols but I did not test that.(2)
If component has
interfaceOnlycodegen config option set totruecodegen does not generate part of the required code, including component descriptor (CD). This is the case forScreenNativeComponent. This leads to its manually created CD not being picked up by autolinking ==> we need to somehow inform RNCLI that this CD exists. This is done by specifyingcomponentDescriptorsoption inreact-native.config.jsfile. AFAIK it does not allow to extend set of codegen'd CDs, it only allows to overwrite it, hence we need to specify all CDs manually.Test code and steps to reproduce
Build any of FabricExample applications in the repository (or create fresh React Native app) and see that they work properly
Checklist