Open karthiktrcapgemini opened 1 year ago
@cipolleschi can you help here?
@ravindraguptacapgemini @karthiktrcapgemini thanks for testing this.
I don't have an immediate answer, as this error looks a bit hard to identify for me. Can you prepare a repro using this template?
This will make it easier for us to run it and see what's going wrong. Thank you so much!
cc. @cortinico as this is also failing for Android
Sure @cipolleschi, We will share a reproducible repo as soon as possible.
@cipolleschi - Here is the reproducible repo for this -> https://github.com/karthiktrcapgemini/react-native-sketch-canvas-interop-layer-issue
Adding a few more details -
Thanks for sharing this. For full transparency, I'm going to be on PTO for some days starting from the next week. I expect to be back and operational from the 19th. Can this wait before I'm back?
Unfortunately, We are in the process of migrating one of our apps to new arch and this is one of the few blockers we are facing. Any way at all to expedite this or things we could check at our end in the meantime? @cipolleschi
ok, I found some time to investigate this for iOS.
I think that the root problem is in the logic of how the views are added/removed from the registry. Right now, we add a view to the legacyView registry right before a command is dispatched and we remove it right after.
The problem is that RNSketchCanvas expect the view to always be in the registry, so it tries to access the registry outside a command invocation (due to some async call) and that fails.
I prepared this PR that should fix the issue on iOS, but we have to wait for @cortinico for Android.
Thanks a lot @cipolleschi, this sounds great. I will keep this thread posted once I get a chance to test this fix for iOS
We are in the process of migrating one of our apps to new arch and this is one of the few blockers we are facing
I just want to highlight a thing. Although I really really appreciate the effort in migrating your app to the New Architecture, and all the feedback that comes with the migration, please remember that the New Architecture is still experimental. There will be breaking changes in the APIs while we work in releasing everything that's needed and there will be new pieces of the Architecture that will arrive in 0.73.
Thanks @cipolleschi. Acknowledged. Also, The fix seems to be working fine for iOS but we will still need a bit more scenario testing on it. @cortinico - Your inputs for Android would be greatly appreciated.
@cortinico we are getting this issue for multiple component, sketch-canvas is one of them, can you please check the reproducer and see what's breaking thing there. Hope to get your response, thanks.
Hi @karthiktrcapgemini @ravindraguptacapgemini Thanks for sharing a reproducer which helped me understand what was going on.
The problem is with commands & the interop layer. Specifically react-native-sketch-canvas
is using receiveCommand(View, int, ReadableArray)
here:
https://github.com/terrylinla/react-native-sketch-canvas/blob/b322a86dbaccfac7327095e505cf7d7cc9b4acde/android/src/main/java/com/terrylinla/rnsketchcanvas/SketchCanvasManager.java#L93
I would need to wait for @cipolleschi to be back in the office next week to discuss this, but I noticed a change for iOS: https://github.com/facebook/react-native/commit/c7aaae61ca259a546331aca3cce475ad981b1b22 is causing all events to be delivered as string
, which ends up not delivering those events on this specific component.
We'll investigate further and get back with a fix as this is definitely affecting other libraries.
Hi @cortinico Thanks for identifying the RC. Is there a tentative ETA on the PR or version with which the fix for this might be released?
Hi @cortinico Thanks for identifying the RC. Is there a tentative ETA on the PR or version with which the fix for this might be released?
Hey @ravindraguptacapgemini @karthiktrcapgemini The fix will be shipped when it's ready
Hi @cortinico, Thanks for releasing the above Android issue fix for this as part of 0.72.4. We have had the chance to test the fix and see an additional issue only with Android. This issue is completely fixed now with iOS with interop layer. Details below -
0.72.4
Android
Android Pixel 5 (API 33)
We are now able to sketch on the canvas which we were previously unable to do as per our above post. But now, getBase64* method does not return the result base64 we need for the sketch. This works perfectly fine for iOS. Please see SSs below -
*From the Documentation - getBase64(imageType, transparent, includeImage, cropToImageSize, callback) | Get the base64 of image and receive data in callback function, which called with 2 arguments. First one is error (null if no error) and second one is base64 result.
https://github.com/karthiktrcapgemini/react-native-sketch-canvas-interop-layer-issue
Hi @karthiktrcapgemini Thanks for updating the repro, this allowed me to test really quickly and understand what's the issue.
The problem is that the getBase64
method is implemented differently between Android & iOS:
https://github.com/terrylinla/react-native-sketch-canvas/blob/b322a86dbaccfac7327095e505cf7d7cc9b4acde/src/SketchCanvas.js#L147-L153
On iOS it delegates the implementation to a method in the ViewManager. On Android instead it uses a separate Native Module.
Calling NativeModules.SketchCanvasModule.(...)
is currently not supported on the New Architecture. We're working on providing a Native Module Interop Layer in 0.73 that will most likely fill this gap, but for the time being, this library should be patched to don't do this.
In an ideal world this library should instead fire a command to save to Base64 that the ViewManager can consume and work on.
I took a look at a patch for this and I learned that the methods were built differently to workaround getting a value directly from the ViewManager.
So the SketchCanvasModule just grabs the view of the SketchCanvas to call a method off it, in order to return it in a callback.
Based on @cortinico comment - this probably was not the way to do things. I just struggle to figure out if this plugin was always just abusing a method of development or if it should have always hit a command to view manager for it to emit an event back.
Since lets say we make an event/callback on the View manager for like onSave
and onBase64
. It makes perfect sense for onSave
since you are providing output about an operation that completed or didn't.
Saving base64 is just getting an output scalar back, so an async nature of making a command and subscribing to an event (onBase64
) seems pretty rough. However, when you consider the current method has to include its own callback in userland - thats also pretty rough.
https://github.com/sourcetoad/react-native-sketch-canvas/blob/master/example/src/App.tsx#L317-L328
Summary
Hi @terrylinla, We have identified that this library is not working with the RN new architecture. So we have moved forward with RN 0.72.1 to enable this legacy module with the interop layer and new arch as suggested here -> https://github.com/reactwg/react-native-new-architecture/discussions/135
Reproducible sample code -
import React, { Component } from 'react'; import { AppRegistry, StyleSheet, View, } from 'react-native';
Steps to reproduce
Expected result -
Actual Result -
-Module opens and we are able to add a sketch but then it throws this exception -
React Native Sketch Canvas Version
0.8.0
What platforms are you seeing the problem on?
Android, iOS
React Native Version
0.72.1
What version of Expo are you using?
Not using Expo
Device(s)
iPhone 14 (iOS 16.4) | Android Pixel 5 (API 33)
Additional information
No response