-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
chore: restore backward compat for new & old architecture #2730
Conversation
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.
Notes for future me.
internal class DimmingView( | ||
context: Context, | ||
initialAlpha: Float = 0.6F, | ||
private val pointerEventsProxy: DimmingViewPointerEventsProxy | ||
) : ViewGroup(context), | ||
ReactCompoundViewGroup, | ||
ReactPointerEventsView { | ||
private val blockGestures | ||
ReactPointerEventsView by pointerEventsProxy { | ||
|
||
constructor(context: Context, initialAlpha: Float = 0.6F) : this( | ||
context, initialAlpha, | ||
DimmingViewPointerEventsProxy(null) | ||
) |
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.
Doing it it such convoluted way, because I want the interface implementation to be delegated, but the implementation requires reference to this@DimmingView
which can not be passed while calling other constructor.
This should not cause any timing problems since I'm initialising the implementation field just below in the init
block.
Delegation of the interface implementation allows me to not put whole logic under versioning but rather only its fragments related to pointer events.
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.
Unused code that would required to be adjusted similarly to DimmingView & ScreensCoordinatorLayout.
#if defined __has_include | ||
#if __has_include( \ | ||
<React-RCTAppDelegate/RCTReactNativeFactory.h>) // added in 78 | ||
#define RNS_REACT_NATIVE_VERSION_MINOR_BELOW_78 0 | ||
#else | ||
#define RNS_REACT_NATIVE_VERSION_MINOR_BELOW_78 1 | ||
#endif | ||
#endif |
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.
This is not pretty, but good enough for now.
If we need to have more RN version dependent code in the future we should define RNS_REACT_NATIVE_VERSION_MINOR
during compilation or something.
I've also seen that REACT_NATIVE_VERSION_MINOR
macro is defined in ReactNativeVersion.h
since RN 0.79.
/** | ||
* Adapts files codegend for the new architecture for paticular needs of old architecure. | ||
* This usually comes down to keeping backward compat on old architecture. | ||
* | ||
* @param {string} dir - directory with codegened files | ||
*/ | ||
function fixOldArchJavaForRN77Compat(dir) { | ||
console.log(`${TAG} fixOldArchJavaForRN77Compat: ${dir}`); | ||
const files = readdirSync(dir); | ||
files.forEach(file => { | ||
const filePath = path.join(dir, file); | ||
const fileExtension = path.extname(file); | ||
if (fileExtension === '.java') { | ||
const fileContent = fs.readFileSync(filePath, 'utf-8'); | ||
let newFileContent = fileContent.replace( | ||
/extends ViewManagerWithGeneratedInterface/g, | ||
'', | ||
); | ||
if (fileContent !== newFileContent) { | ||
// Also remove redundant import | ||
newFileContent = newFileContent.replace( | ||
/import com.facebook.react.uimanager.ViewManagerWithGeneratedInterface;/, | ||
'', | ||
); | ||
|
||
console.log(' => fixOldArchJava77 applied to:', filePath); | ||
fs.writeFileSync(filePath, newFileContent, 'utf-8'); | ||
} | ||
} else if (fs.lstatSync(filePath).isDirectory()) { | ||
fixOldArchJavaForRN77Compat(filePath); | ||
} | ||
}); | ||
} |
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.
78 introduced ViewManagerWithGeneratedInterface
that codegened interfaces extend.
This causes issues on RN versions <= 77. However it seems to not be required for Paper to work properly on 78, therefore I decided to remove it.
e2e fails because atm reanimated v4 is set in example apps (as there is no reanimated v3 with support for 78 yet) and it does not work on old architecture. |
…events (#2740) ## Description Regression introduced in 4.9.0, when adding backward compatibility in #2730. I've inverted the logical condition checking whether dimming view blocks gestures and returned `PointerEvents.AUTO` when it wasn't supposed do block. This PR fixes the condition. Fixes #2738 ## Test code and steps to reproduce Enhanced `TestFormSheet` so that it allows to verify that it works correctly now. ## Checklist - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes
Restoring backward compatibility for old architecture - now we require 76. 75 could work, but I've not tested this. On new architecture we require 77. It should compile on 76, however we need the fix for the removal of transitioning views that landed in 77. See here: #2596. Described mostly down below 👇🏻 in review comments. I've tested this manually on fresh RN 76 & 78 app, new/old architecture, iOS/Android. - [ ] Ensured that CI passes (cherry picked from commit b5a0f9f)
## Description Adding support for `react-native@0.78.0`. ## Changes - adjusted example apps (Android, iOS) on both architectures (Paper, Fabric) with changes from upgrade helper, - added additional fixes (android, ios, cpp) from 4.x (#2626), - updated dependencies, gradle of the library: - uncommented `syncArchs` task, because updating `react-native` version of the library fixed the problem with codegen, - fixed issue connected with Gesture Handler: - #2618, - updated TVOS example app (from 0.74.1) to match example app from 4.x and Android, iOS apps, - fixed a bug with `replaceNavigationBarViewsWithSnapshotOfSubview` method by adding a nil check: - adapted from #2485, - restoring backward compatibility: - #2730, - #2737. ## Test code and steps to reproduce CI ## Checklist - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Description
Restoring backward compatibility for old architecture - now we require 76. 75 could work, but I've not tested this.
On new architecture we require 77. It should compile on 76, however we need the fix for the removal of transitioning views that landed in 77. See here: #2596.
Changes
Described mostly down below 👇🏻 in review comments.
Checklist
I've tested this manually on fresh RN 76 & 78 app, new/old architecture, iOS/Android.