Skip to content

Conversation

@mfazekas
Copy link
Contributor

@mfazekas mfazekas commented Nov 16, 2023

Description

Fixes building against react-native 0.73.0-rc4 in fabric mode on android

Changes

Updated common/cpp/react/renderer/components/rnscreens/RNSScreenComponentDescriptor.h to build with fabric on 0.73-rc4.

Screenshots / GIFs

Before

After

Test code and steps to reproduce

npx [email protected] init rnscreenstest
yarn add react-native-screens
cd android
# change newArchEnabled to true in `gradle.properties`
gradlew build

Checklist

@mfazekas mfazekas mentioned this pull request Nov 16, 2023
5 tasks
@kkafar
Copy link
Member

kkafar commented Nov 16, 2023

Hey, I've not tested it throughly yet, however this seems like not backward compatible change.
In case of Fabric, we do not care for backward compat RN, but it looks like it could break Paper backward compatibility and we can not afford it for now. I'll work on adding some compile-time constants for C++ code with RN major, minor & patch versions so that we can compile the right code for the right version (is there different solution)?

@mfazekas
Copy link
Contributor Author

@kkafar Yes it's a breaking change, this only affected fabric builds, at least in our CI.

At least on 0.73 this file was not part of the build when building for paper, only when building fabric.

image

https://github.com/rnmapbox/maps/actions/runs/6886704342

I've also mentioned this at reactwg/react-native-releases#64 (comment), if there is a C++ macro we can already use. We can probably use Gradle to figure out RN versions and push that to C++.

@kkafar
Copy link
Member

kkafar commented Nov 16, 2023

This is rather unexpected, this file should be compiled (included into compiled file) during Paper build. If it is not the case, we are good, as we do not need to care for Fabric backward compatiblity.
I'll wait for our CI & test in later on some local branch.

PS. Thanks for contributing!

@kkafar kkafar changed the base branch from main to @kkafar/rn-73 November 24, 2023 11:03
@kkafar kkafar merged commit 3849e2c into software-mansion:@kkafar/rn-73 Nov 27, 2023
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