-
-
Notifications
You must be signed in to change notification settings - Fork 850
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
Add new architecture support #3022
Conversation
Hi @mfazekas! So far I've migrated the map component, fully only on iOS, (some approaches are inspired by the existing PR) and I have a few things I want to ask about:
If you can spare some time, could you go over the changes in the PR to check if you can see any problems with the approaches taken? It would be much easier to solve them now, given that relatively not much has been done yet 😅. |
Thanks this is awesome! Yes MBX should be ok, for prefixes. And I think it's fine to use only when necessary. We can clean that up later. Native modules is a clever workaround for the codegen limitations I've also faced. For the first glance your approach worked, I'll need to test if it works on pre fabric world as well. The fabriceexample can stay, we'll see in the future. For now my feeling is that they make the RN upgrade more complicated, than the value it adds, but we'll see it should be easy to remove at any time. I'll try to go over the changes in more details over the weekend. |
I haven't tested the old architecture with the changes applied yet, as was focusing on getting the new arch to work fully. I don't think it will work at the moment 😅. |
override fun onMapLoadError(mapLoadingErrorEventData: MapLoadingErrorEventData) { | ||
Logger.w("MapLoadError", mapLoadingErrorEventData.message) | ||
} | ||
object : OnMapLoadErrorListener { |
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.
It's a bit unfortunate to have both whitespace changes and significant changes in the same PR, it's make kind of pain to review.
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.
Sorry, I don't remember changing it. They should be removed now.
fun getCoordinateFromView(callbackID: String?, pixel: ScreenCoordinate) { | ||
getCoordinateFromView(pixel) { | ||
sendResponse(callbackID, it) | ||
} | ||
} |
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'd put this to the caller, which would be then:
- mapView.getCoordinateFromView(args!!.getString(0), args.getArray(1).toScreenCoordinate());
+ mapView.getCoordinateFromView(args.getArray(1).toScreenCoordinate()) { sendResponse(args!!.getString(0), it};
Actually I've refactored android commands based on your changes so they can be used as react methods as well - see #3029. Sorry it'll be a bit of pain to rebase your changes
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've rebased the PR to the main
branch. Do you think merging instead of rebasing would be a problem in the future? It would be easier to do, and it would simplify parallelizing the work.
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.
Look great, thanks much for all the work. Feel free to use merge instead of rebase if you prefer that.
- codegen partially works - map is actually displayed - *one* prop is working
@mfazekas @WoLewicki figured out a clever way to use the swift header by adding a forward declaration for the We also figured that since there is a native module on the new arch, we can use it instead of commands on the old arch as well to reduce the complexity and code duplication. Could you look at the changes and tell us what you think? |
Thanks, clever. Yes it's look simpler this way.
NativeModule is something I wanted to implement on Android, so that is great. |
It looks like e2e tests are failing for the
It looks like |
Yep CI cannot run Mapbox as metal is not supported on GitHub CI actions. So we've used Mapblibre for the CI. But now Maplibre is so far behind we can just remove it. |
So what exactly are the next steps here? |
I guess for me to review and merge, disable MapLibre CI, so we can go forward |
Ah, I completely missed this 😅. We tried it (only the |
@j-piasecki I have a strange issue with this branch. In the PointInMapView.js example. The async method call
here only resolved every second time (but then the old one resolves as well.) It seems that the culprit is [UIManager addUIBlock:^()]. Queues the block but then execution get delayed to the second click. I think the issue is that MBXMapViewModule was previously RCTMGLMapViewManager which was a subclass of RCTViewManager. And I think addUIBlock cannot be used outside RCTViewManager, in the sense that the block might be just queue, but the queue might be only flushed by the next ui operation. note This is the recording of rnmapbox example from the fabric branch. Note that coords are updated every second call. This is the recording of rnmapbox example from main branch. |
@j-piasecki Some minor notes, none of those requires any immediate action more a FYI: 1.) actually the MBXMapView is not really good as the native Mapbox libraries also use the MBX prefix. So we should be using RNMBXMapView or something like that. But we can change that later. Sorry for disinformation about that. 2.) Xcode seems to be giving errors in the IDE about |
ios/RCTMGL-v10/MBXMapViewModule.mm
Outdated
if (view != nil) { | ||
block(view); | ||
} else { | ||
reject(@"takeSnap", [NSString stringWithFormat:@"Unknown reactTag: %@", viewRef], nil); |
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.
reject(@"takeSnap", [NSString stringWithFormat:@"Unknown reactTag: %@", viewRef], nil); | |
reject(@"MBXMapView command", [NSString stringWithFormat:@"Unknown reactTag: %@", viewRef], nil); |
We could also pass the command names for more precise error. But I don't think that's worth the effort.
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.
done ✅
@mfazekas I pushed a change that should fix the issue you have in #3022 (comment). Could you test it? For some reason the ui blocks are dispatched before the one we add is inserted in the queue, so I changed them to be added on |
Should we change it in this PR or can it be done later? |
It can be done later. Was more a reminder to myself. |
@j-piasecki thanks much for all the great work. I've merged to main now. Created a branch for |
Ok, we've already migrated many more of the views so we'll keep submitting the PRs with them 🚀 |
@j-piasecki how do we update generated files in |
When running the app on the new arch, those files should be generated in the main lib's |
Makes sense, thanks. What is the plan for the We used to pass it as a json object, I think in codegen cannot hand such complex object type. maps/src/utils/MapboxStyles.d.ts Lines 616 to 1043 in f34aa2f
Lines 1071 to 1088 in f34aa2f
|
Yeah, it basically makes a folly:dynamic on the native side that we can parse as we like then. |
This is only available in RN 0.72 right? So we only support 0.72 and later for new architecture builds: With "react-native": "0.71.7", I'm getting:
|
Wow, I didn't even know this, would have to check. And yeah, the current state of Fabric is that it introduces breaking changes in each version of |
Description
Adds setup for the new architecture support to the library:
MapView
class, so that swift header can be used inMapView
component as base for migrationWe will work incrementally to migrate the rest of the components and modules on top of the changes introduced here.
This work is done on behalf of Expensify.
Checklist
yarn generate
in the root folder/example
)Screenshot OR Video