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

WIP: fabric codegen test #2819

Closed
wants to merge 3 commits into from
Closed

WIP: fabric codegen test #2819

wants to merge 3 commits into from

Conversation

mfazekas
Copy link
Contributor

@mfazekas mfazekas commented Apr 26, 2023

Current state:

  • This PR is a prototype to understand what kind of changes are required for Fabric/TurboModules

Issues:

  • looking for sponsor: It's a non trivial task, and I cannot afford it doing from my free time, looking for sponsors.
  • codegen limitations: codegen has severe limitations - like commands cannot accept arrays at the moment. One workaround for this is serialising JSON as string and passing through that way, but it will not be neither pretty nor fast
  • layer style properties generator for fabric - this is a missing thing in the POC

Milestones:

  • iOS initial POC (how to use swift, how to reuse original MapView code)
  • iOS experiment with swift C++ integration to simplify swift usage
  • Android initial POC
  • implement MapView with a property, command and callback [iOS]
  • implement MapView with a property command and callback [Android]
  • implement Camera with a property, that can be added to Map
  • implement a source/layer without styles
  • implement layer with style, generate codes for style * [iOS]
  • implement layer with style, generate codes for style * [Android]
  • implement all layers
  • implement all map view features
  • implement all camera features
  • implement Images/Image component
  • implement userLocation, offlineManager components

Some documentation links:

Things are a bit compilicated as we use swift. And fabric is C++. While Objective-C has great C++ interop via objective-C++, swift C++ interop is still in progress . Swift has great interop with pure objectve-C classes and protocols, but it doesn't have interop with objective-c classes which has c++ members base of fabric component: RCTViewComponentView. Alternative would be implementing RCTComponentViewProtocol directly, but since it's operates on C++ object, it's problematic to porvide swift implementations

flowchart TD
    A[MapView.tsx]-->|"uses(if fabric)"| B
    B[RNMBXMapView.tsx] -->|codegen| C("@rnmapbox_maps/*.h,*.cpp (c++)")
    D["RNMBXMapView.h,.mm (obj-c++)"] -->|includes, implements| C
    D --> |contentView| E["RCTMGMapView (swift)"]
    E --> |implements| F["RNMBXMapViewImplProtocol from RNMBXMapViewImpl.h (obj-c)"]
    D --> |includes, uses| F
    G[RNMBXMapViewImplFactory] --> |creates| E["RCTMGMapView (swift)"]
Loading

Current state:

image

Issues:

@mfazekas mfazekas force-pushed the WIP-fabric-tests branch 2 times, most recently from f6cd2ba to 4ac2235 Compare April 29, 2023 17:20
@mfazekas mfazekas force-pushed the WIP-fabric-tests branch from 4ac2235 to 5d3f304 Compare May 1, 2023 18:25
@j-piasecki
Copy link
Contributor

Hi! Together with @WoLewicki, we would like to help with implementing the new architecture support on behalf of Expensify.

I wanted to ask about android setup - the source files are located under android/rctmgl/ instead of just android/, what is the reason for this? It's a bit problematic with codegen, which looks for package.json with config exactly one directory above the android project (currently it ends up looking for package.json in android/).

@mfazekas
Copy link
Contributor Author

mfazekas commented Aug 25, 2023

@j-piasecki thanks much for looking into it. I don't really see why it's in android/rctmgl instead of using just android.

We can move that to the android. But let's do that on main, before fabric. (To make comparing changes easier.)

@j-piasecki
Copy link
Contributor

@mfazekas Great! I've made a PR doing that - #3011. Could you make sure I didn't miss anything?

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