-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[core] remove view wrapper from ExpoFabricView #28829
Conversation
The Pull Request introduced fingerprint changes against the base commit: ec256db Fingerprint diff[
{
"type": "dir",
"filePath": "../../packages/expo-modules-core",
"reasons": [
"expoAutolinkingIos",
"expoAutolinkingAndroid"
],
"hash": "d66966da890df5f630155fceaeb8a14ab6254a0b"
}
] Generated by PR labeler 🤖 |
assert(appContext == view.appContext) | ||
view.installEventDispatchers() | ||
return view | ||
} |
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.
since we swizzle ViewManagerAdapter_ExpoImage.new() to ImageView.init(appContext:) and we need moduleHolder/viewDefintion in the contructor (for installEventDispatchers()). it doesn't give us chance to inject iMethod or iVar to the ImageView class for the moduleName. this pr tries to add dedicated ExpoFabricView.create() and passing viewDefinition into the class. not a perfect case but works though.
here is the place i'm not happy with. the call flow here is
ViewManagerAdapter_ExpoImage.new()
-> ViewDefinition.createView()
-> ExpoFabricView.create()
-> ImageView.init(appContext:)
-> ExpoFabricView.init(appContext:)
-> set viewDefinition
here.
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.
Yeah, I agree this path doesn't look great. Let's add a comment in the code. We could try to address this later in SDK 52.
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com>
5fa340d
to
5a06058
Compare
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.
I can confirm it works fine, I tested it a bit 😉
a trial to remove the view wrapper from ExpoFabricView 1. let native views inherit ExpoFabricView, so that we don't need ExpoFabricView as a wrapper. this is actually done by `typealias ExpoView = ExpoFabricView`. 2. fabric will call `[viewClass new]` since we need AppContext and ModuleHolder in the initializers, the idea is to swizzle the `new` initializer dynamically. 3. remove `contentView` related logic 4. move `__injectedAppContext` from ExpoFabricViewObjC to ExpoFabricView. that would make the logic clearer because we put the code together. 5. since we swizzle `ViewManagerAdapter_ExpoImage.new()` to `ImageView.init(appContext:)` and we need moduleHolder/viewDefintion in the contructor (for `installEventDispatchers()`). it doesn't give us chance to inject iMethod or iVar to the `ImageView` class for the moduleName. this pr tries to add dedicated `ExpoFabricView.create()` and passing viewDefinition into the class. not a perfect case but works though. --------- Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com> (cherry picked from commit 8421bfb)
Why
a trial to remove the view wrapper from ExpoFabricView
How
typealias ExpoView = ExpoFabricView
.[viewClass new]
. since we need AppContext and ModuleHolder in the initializers, the idea is to swizzle thenew
initializer dynamically.contentView
related logic__injectedAppContext
from ExpoFabricViewObjC to ExpoFabricView. that would make the logic clearer because we put the code together.ViewManagerAdapter_ExpoImage.new()
toImageView.init(appContext:)
and we need moduleHolder/viewDefintion in the contructor (forinstallEventDispatchers()
). it doesn't give us chance to inject iMethod or iVar to theImageView
class for the moduleName. this pr tries to add dedicatedExpoFabricView.create()
and passing viewDefinition into the class. not a perfect case but works though.Test Plan
Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).