Closed cipolleschi closed 6 months ago
Hello @cipolleschi. Thank you for reaching out.
I've already started doing some of the prerequisites, and I'm definitely planning to migrate as soon as my calendar allows me to. I spent some time looking into the migration a couple of weeks ago and wrote down a few notes/questions that I was planning to post in the new architecture workgroup.
I see that the documentation got a major overhaul, so I'll revisit the subject and get back to you if my questions are still relevant after reading the updated docs :)
That's an amazing news! Thank you so much. Feel free to reach out anytime with any question you may have! :D
@cipolleschi I have a couple of questions that I hope you can help sort out :)
Prerequisites for Libraries states that The new renderer also known as Fabric doesnβt use the UIManager so direct calls to UIManager will need to be migrated.
. Is that to be interpreted as any use of UIManager will have to be removed or only the specific cases that the documentation then goes on to describe? react-native-maps
makes use of UIManager.getViewManagerConfig
for instance.
I assume that it isn't, but is native code like:
UIManagerModule uiManager = context.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock(){}
or
[self.bridge.uiManager addUIBlock:^(__unused RCTUIManager *uiManager, NSDictionary<NSNumber *, UIView *> *viewRegistry){}
affected by the above?
Last but not least, react-native-maps
makes use of a provider
prop that determines what native component to load during runtime, meaning a single js/react component doesn't map to a single native component per platform, but to two or, in theory, more.
Does the new arch make any use of the default export from <FABRIC COMPONENT>NativeComponent.ts
or would it be possible to export multiple native components? E.g.:
export const AirMap = codegenNativeComponent<NativeProps>(
'AirMap'
) as HostComponent<NativeProps>;
export const AirGoogleMap = codegenNativeComponent<NativeProps>(
'AirGoogleMap'
) as HostComponent<NativeProps>;
or would it be better to create two <FABRIC COMPONENT>NativeComponent.ts
, one for each native component, even though they are used by the same js/react component and share the same NativeProps and NativeCommands? Or do you have an entirely different solution?
Hi @Simon-TechForm. I made some test on the second question, and unless I did something very wrong, it does not seem to work.
My tests were:
<FABRIC COMPONENT>NativeComponent.js
with two exports => the codegen runs but it is not able to generate any components.<FABRIC COMPONENT>1NativeComponent.js
and a <FABRIC COMPONENT>2NativeComponent.js
importing those props => The parsers fails to find the proper type for the props and fails trying to generate them.So, the output is that, currently, the only way to generate two components with the same props is to create two separate <FABRIC COMPONENT 1>NativeComponent.js
and <FABRIC COMPONENT 2>NativeComponent.js
files, copying and pasting the props.
That's far from ideal, but we plan to improve the Codegen a little, so I hope we would be able to bring this scenario to life.
For your first question I have to dig more internally. I hope to get back with an answer as soon as possible.
Will this fix web support for Expo SDK 46? π₯Ί
Uncaught TypeError: react_native_1.UIManager.getViewManagerConfig is not a function
at ../node_modules/react-native-maps/lib/decorateMapComponent.js (decorateMapComponent.js:29:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ../node_modules/react-native-maps/lib/MapMarker.js (MapMarker.js:29:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ../node_modules/react-native-maps/lib/index.js (index.js:37:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ./routes/ride/FindBeep.tsx (Report.tsx:66:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ./navigators/Drawer.tsx (index.js:8:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ./App.tsx (App.tsx:1:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at ./index.js (index.js:1:1)
at __webpack_require__ (bootstrap:789:1)
at fn (bootstrap:100:1)
at 1 (useValidationErrors.tsx:38:1)
at __webpack_require__ (bootstrap:789:1)
at bootstrap:856:1
at bootstrap:856:1
Hello @Simon-TechForm
The linked paragraph talks about JavaScript specifically. iOS and Android have their dedicated sections.
When writing components in the new architecture, UIManager won't be accessible on the host platform. For example on iOS inside your component, there is no way to get hold of UIManager and other mechanisms have to be used.
What do you need view configs for? We can try to find a way to achieve the same functionality in the new architecture.
Hello @cipolleschi & @sammy-SC. Thank you both for getting back to me.
@cipolleschi thank you for running the tests. I'm not surprised by the results, and I get why it would "have" to behave like that. While the first case seems like an edge case, I could see the second case, importing props from another file, being useful outside of react-native-maps
though :)
@sammy-SC UIManager.getViewManagerConfig
is currently only being used for convenience. As requireNativeComponent
will run and return a string for non-existing native views, getViewManagerConfig
is used to print a warning and return null
for the component instead of crashing the app when trying to render the non-existing native view (installing Google Maps is optional on iOS in react-native-maps
). But given how early in the dev cycle the error will show itself, and how clear the error message is, I'm honestly fine just removing the check.
One other thing: Quite a few methods that are esentially NativeCommands for the NativeComponent was done as NativeModule methods to allow for async methods that could be resolved/rejected with a value. This is currently done by passing a view tag to the NativeModule method by passing findNodeHandle(this.ref.current)
as the first argument, much like we used to do with dispatchViewManagerCommand
before migrating to codegenNativeCommands
. With findNodeHandle
being out of the picture, can you recommend some other way (or docs) to do this? I currenly have a working example that passes this.ref.current._nativeTag
instead, but I can imagine that accesing what I assume is meant to be a private member has both your eyebrows twitching violently π¬ π
Hi @Simon-TechForm! Sorry for the late answer.
I could see the second case, importing props from another file, being useful outside of react-native-maps though :)
Yes, I agree. That's an interesting use case and can lead to much better code. I hope to find some time to deliver this soon.
This is currently done by passing a view tag to the NativeModule method by passing findNodeHandle(this.ref.current) as the first argument, much like we used to do with dispatchViewManagerCommand before migrating to codegenNativeCommands. With findNodeHandle being out of the picture, can you recommend some other way (or docs) to do this? I currenly have a working example that passes this.ref.current._nativeTag instead, but I can imagine that accesing what I assume is meant to be a private member has both your eyebrows twitching violently π¬ π
XD Yep, access a private APIs should be avoided.
So, the proper way to replace findNodeHandle
is with forwardRefs
for function components and with a getter
for class components.
That said, we are working on ways to simplify the migration and we are considering keeping around the findNodeHandle
API for a while. This should give you and other library maintainer some more time to migrate! ;)
The reason why we are deprecating it is because its behavior is unpredictable with some new features of React18. However, this features are opt-in and therefore, it should be safe to still use findNodeHandle
if you don't adopt them (however, you'll never know if an app importing your library will wrap your component in some of the new features... So it is much safer to migrate them).
Hi @cipolleschi. Don't worry :)
Problem is, we are passing it to the native side through NativeModule
methods, so I can't really use either of them.
E.g:
NativeModules.AirMapModule.coordinateForPoint(findNodeHandle(this.map.current), point);
and then on the native side we are doing (android as an example):
@ReactMethod
public void coordinateForPoint(final int tag, ReadableMap point, final Promise promise) {
...
UIManagerModule uiManager = context.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock()
{
@Override
public void execute(NativeViewHierarchyManager nvhm)
{
AirMapView view = (AirMapView) nvhm.resolveView(tag);
...
});
}
to get a reference to the view on the native side. This is currently the only use we have for findNodeHandle
, so I'd prefer migrating as well :)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@Simon-TechForm Are you still working on the new arch?
@Simon-TechForm Hey, hope you are doing well :) Like @midrizi, I also wanted to see if there was an update around the usage of the new architecture? Is there a place where pending tasks are listed? More than happy to contribute!
@midrizi @kalyantm I've had very limited time to spend on react-native-maps in the previous months, but the time I had, I spent on supporting the new architecture. I've had no "behind the scenes" communication with the RN team, so the issue I mentioned in my latest comment is very much still applicable. But since findNodeHandle
seems to stick around, I've moved on to the next step(s).
I've just applied for access to the new architecture working group as I've stumbled upon a couple of features that doesn't seem to be supported when codegenerating typescript specs in the new architecture and am currently awaiting access :)
and am currently awaiting access :)
You should have access by now π
For me it was adding the two lines below.
import {enableLatestRenderer} from 'react-native-maps'; enableLatestRenderer();
@uwakahuche Where did you specify that? I still get Unimplemented component <AIRGoogleMap>
@kalyantm App.js
@cortinico Thank you and apologies for the late reply :)
I've just commented in the Codegen Missing features discussion, raising the issues that's currently blocking migration of react-native-maps
.
Hi!
Thank you for this awesome library! It's really one of the most helpful I've ever used on RN :D
Is there any news regarding this issue ? I'm currently working with RN 0.71.2 with the new Architecture and I really miss react-native-maps π¬ I forked @michbil's fork (https://github.com/michbil/react-native-maps/tree/feature/new-architecture-exp) where he tried to add support for the new architecture, and I got it working on iOS but not on Android yet, and my knowledge of native code is...well, not enough sadly! I just tried to command some stuff but of course I suppose this is not enough even though it compiles and run on dev env for iOS π (https://github.com/michbil/react-native-maps/tree/feature/new-architecture-exp)
Is the anything I can do to help make react-native-maps support the new architecture ?
I have figured it out for iOS . Check this https://www.youtube.com/watch?v=-08s1kr1SBE
Any updates? We still earnestly waiting for it π₯Ή
I am also willing to help. I am author of this summer hack(https://github.com/michbil/react-native-maps/tree/feature/new-architecture-exp) to make library work on new architecture iOS. @Simon-TechForm Is there any branch where help is needed?
I have figured out how to display google maps for both android as well, if interested check it out https://www.youtube.com/watch?v=ZpYDzLGJTXY
A quick update for everyone interested. The status of this is the same as mentioned in https://github.com/react-native-maps/react-native-maps/issues/4383#issuecomment-1435786037 :)
@Simon-TechForm we are migrating our app to new architecture, Is there any quick workaround to add support for fabric. Please suggest.
@ravindraguptacapgemini you can try the new interop layer https://github.com/reactwg/react-native-new-architecture/discussions/135 but I'm not sure if it will work with react-native-maps
yes, please! If any of you had chance, let's try to use the interop layer and report failures. We are actively working on that and any feedback is super valuable. We are committing on fixing most of the basic failures to try and support most of the use cases with the interop layer!
@cipolleschi thanks for the update. I have tried Interop layer with 0.72.0 with react-native-maps on iOS, specified this in react-native-config.js
module.exports = {
project: {
android: {
unstable_reactLegacyComponentNames: ['AIRGoogleMap'],
},
ios: {
unstable_reactLegacyComponentNames: ['AIRGoogleMap'],
}
},
};
It crash on load the map, sharing part of crash report here:
Incident Identifier: 7B53C949-83B7-4B42-A5B0-39AE5F2F933A CrashReporter Key: 6E546EFD-FB91-747D-EE9D-6F28E9A1CB76 Hardware Model: Mac14,10 Process: intelexMobile [4241] Path: /Users/USER/Library/Developer/CoreSimulator/Devices/CAB004FE-326E-4CAE-8167-3CEEE227C70C/data/Containers/Bundle/Application/D9F5B09C-1A1F-411D-A2E9-0BFFE049988C/intelexMobile.app/intelexMobile Identifier: com.intelex.intelexMobile Version: 1.0 (1.0) Code Type: X86-64 (Native) Role: Foreground Parent Process: launchd_sim [2229] Coalition: com.apple.CoreSimulator.SimDevice.CAB004FE-326E-4CAE-8167-3CEEE227C70C [1448] Responsible Process: SimulatorTrampoline [2133]
Date/Time: 2023-06-30 21:21:46.7224 +0530 Launch Time: 2023-06-30 21:18:31.0195 +0530 OS Version: macOS 13.4 (22F66) Release Type: User Report Version: 104
Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Triggered by Thread: 6
Thread 0:: Dispatch queue: com.apple.main-thread
0 ??? 0x1058539a8 ???
1
Thread 1:: com.apple.rosetta.exceptionserver 0 ??? 0x7ff7ffbd6694 ???
Thread 2:: com.apple.uikit.eventfetch-thread
0 ??? 0x1058539a8 ???
1
Thread 3:: KSCrash Exception Handler (Secondary)
0 ??? 0x1058539a8 ???
1
Thread 4:: KSCrash Exception Handler (Primary)
0 ??? 0x1058539a8 ???
1
Thread 5:: com.bugsnag.app-hang-detector
0 ??? 0x1058539a8 ???
1
Thread 6 Crashed:: com.facebook.react.JavaScript
0
@ravindraguptacapgemini thanks for testing that. For 0.72.0, we are aware of a bug in the iOS interop layer, for which legacy components with ViewManagers not starting with the RCT
prefix were crashing the app.
The bug has been fixed in 0.72.1, so now that should be ok. Commit for reference.
Let me know if you have time to try with 72.1! :D
@cipolleschi thank you so much, I tried with React Native 0.72.1, and it did work fine for iOS but somehow it's breaking up for android, the map loads with some error, here is the error screenshot. Please check of you can help here.
@ravindraguptacapgemini thank you so much for testing this! In your message, you wrote:
I tried with React Native 0.72.1, and it did work fine for android but somehow it's breaking up for android
I guess that the first android was actually ios, as the image you linked with the error is of an Android device.
@cortinico this seems to be a problem with the events (surprise surprise).
@ravindraguptacapgemini can you create a repro using this repo?
@cipolleschi @cortinico can we expect this issue to be fixed in coming version (may be an RC) of React Native, also please let me know, if there is any timeline. thanks
Very likely yes. Events are one of the basic features of React Native and the interop layer, so we would like to have it fixed in the interop layer. But to fix this effectively, we are going to need a reliable repro
@cipolleschi I have created a repro for you, please check it out from here.
@cortinico βοΈ
@cortinico hey man!, have you got a chance to check for this issue?
Hey @ravindraguptacapgemini Please don't ping us over the weekend. I'll check that once we're back in the office next week. Thank you
Hey, also face this issue on android, find any solution for it?
Hey @ravindraguptacapgemini Please don't ping us over the weekend. I'll check that once we're back in the office next week. Thank you
@cortinico sorry to ping here on weekend, hope now you are in office and just curious to if by any chance you checked on this issue. Thanks
We are facing the unsupported event issue as well for Android. Is there a solution for this?
Hi @ravindraguptacapgemini,
Thanks for providing a reproducer, this was really valuable for me. I identified a scenario in the interop layer where direct (non-bubbling) events, like the one fired from react-native-maps
are not properly handled. I'll work on a fix, and this should hopefully land in the next point release of 0.72
The fix is here:
I'll have it reviewed and merged in the upcoming stable for 0.72
@cortinico really appreciate your effort to check for the issue. I have tried to apply the changes from the PR on my local, but they do not work, still getting the same error. I even tried to replace contents of the file as there were other changes as well, nothing changed. Could you please suggest how to check this fix on local. Also please let me know if there is any tentative date for the next stable release of 0.72. Thanks again!
@cortinico Thanks for your effort. Trying this changes locally with react-native 0.72.3 but facing same issue on it.
This change is not released yet. It will be included in 0.72.4
@Simon-TechForm please update if you are now planning the add new architecture support for the library.
@cortinico We are also observing the map to crash on re-render in some case with new architecture, here is the stack-trace:
Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Triggered by Thread: 0
Last Exception Backtrace:
0 CoreFoundation 0x11b4aa77b exceptionPreprocess + 226
1 libobjc.A.dylib 0x112b51b73 objc_exception_throw + 48
2 Foundation 0x1190fe109 -[NSMutableDictionary(NSMutableDictionary) classForCoder] + 0
3 intelexMobile 0x10512ad41 -[RCTComponentViewRegistry _enqueueComponentViewWithComponentHandle:componentViewDescriptor:] + 721 (RCTComponentViewRegistry.mm:115)
4 intelexMobile 0x10512a1bc -[RCTComponentViewRegistry enqueueComponentViewWithComponentHandle:tag:componentViewDescriptor:] + 940 (RCTComponentViewRegistry.mm:70)
5 intelexMobile 0x10516d921 RCTPerformMountInstructions(std::1::vector<facebook::react::ShadowViewMutation, std::1::allocator
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0
Thread 1:: com.apple.rosetta.exceptionserver 0 ??? 0x7ff7fff95694 ???
Thread 2:: com.apple.uikit.eventfetch-thread
0 ??? 0x109a9f9a8 ???
1
Thread 3:: KSCrash Exception Handler (Secondary)
0 ??? 0x109a9f9a8 ???
1
Thread 4:: KSCrash Exception Handler (Primary)
0 ??? 0x109a9f9a8 ???
1
This is an iOS crash, so more on my plate rather than @cortinico.
Can you set up a repro for this crash so that I can investigate it properly?
@cipolleschi I was not able to add it to the reproducer app. It happens when providing custom icon component for marker, like:
<Marker coordinate={{latitude: latitude, longitude: longitude}}>
<MyCustomMarkerView {...marker} />
</Marker>
Please check if updated log can help:
Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Triggered by Thread: 0
Last Exception Backtrace:
0 CoreFoundation 0x11b9df77b exceptionPreprocess + 226
1 libobjc.A.dylib 0x113086b73 objc_exception_throw + 48
2 CoreFoundation 0x11b9ee8c4 +[NSObject(NSObject) instanceMethodSignatureForSelector:] + 0
3 UIKitCore 0x12ad61822 -[UIResponder doesNotRecognizeSelector:] + 264
4 CoreFoundation 0x11b9e3c66 __forwarding + 1443
5 CoreFoundation 0x11b9e5e08 _CF_forwarding_prep_0 + 120
6 intelexMobile 0x10595b99d -[AIRGoogleMapMarker removeReactSubview:] + 61 (AIRGoogleMapMarker.m:100)
7 intelexMobile 0x1056cea6d -[RCTLegacyViewManagerInteropComponentView unmountChildComponentView:index:] + 157 (RCTLegacyViewManagerInteropComponentView.mm:163)
8 intelexMobile 0x1056e752f RCTPerformMountInstructions(std::1::vector<facebook::react::ShadowViewMutation, std::1::allocator
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0
Thread 1:: com.apple.rosetta.exceptionserver 0 ??? 0x7ff7ffd73694 ???
Thread 2:: com.apple.uikit.eventfetch-thread
0 ??? 0x109fd49a8 ???
1
Thread 3:: KSCrash Exception Handler (Secondary)
0 ??? 0x109fd49a8 ???
1
Thread 4:: KSCrash Exception Handler (Primary)
0 ??? 0x109fd49a8 ???
1
Thread 5:: com.bugsnag.app-hang-detector
0 ??? 0x109fd49a8 ???
1
Thread 6:: com.apple.NSURLConnectionLoader
0 ??? 0x109fd49a8 ???
1
have you added also MyCustomMarker
to the interop layer?
Summary
Hi @Simon-TechForm, I'm a software engineer at Meta, working on React Native. We notice that your library is really popular and we would like to know if you are considering migrating it to the New Architecture, using the Fabric Renderer and TurboModules. You can reed more about the New Architecture here, and here you can find a guide on how to migrate the library.
Are there any plans to proceed with the migration? Can we support you in any way?
Reproducible sample code
Steps to reproduce
npx react-native init TestMaps --version 0.70.0-rc.2
yarn add react-native-maps
cd ios
bundle install
RCT_NEW_ARCH_ENABLED=1 bundle exec pod install
cd ..
android/gradle.properties
newArchEnabled
boolean fromfalse
totrue
npx react-native run-ios
npx react-native run-android
Expected result
A properly loaded map.
Actual result
A screen with pink background saying Unimplemented component:
React Native Maps Version
1.2.0
What platforms are you seeing the problem on?
Android, iOS (Apple Maps), iOS (Google Maps)
React Native Version
0.70.0-rc.2
What version of Expo are you using?
Not using Expo
Device(s)
iPhone 13 (iOS 15.5) | Android Pixel 5 (API 33)
Additional information
No response