Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented May 14, 2024

Why

a trial to remove the view wrapper from ExpoFabricView

How

  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.

Test Plan

  • fabric-tester
  • bare-expo

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 14, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented May 14, 2024

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
}
Copy link
Contributor Author

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.

Copy link
Member

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.

}
}

/**
Calls lifecycle methods registered by `OnViewDidUpdateProps` definition component.
*/
public override func viewDidUpdateProps() {
guard let view = contentView, let viewDefinition = moduleHolder?.definition.view else {
guard let viewDefinition = viewDefinition else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can skip it if the name is the same.

Suggested change
guard let viewDefinition = viewDefinition else {
guard let viewDefinition else {

Kudo and others added 2 commits May 16, 2024 14:16
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com>
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 16, 2024
@Kudo Kudo force-pushed the @kudo/remove-fabric-view-wrapper branch from 5fa340d to 5a06058 Compare May 16, 2024 06:17
Copy link
Member

@tsapeta tsapeta left a 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 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants