react-native-sketch-canvas does not support new architecture even with RN v0.72 interop layer

[karthiktrcapgemini]

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 -> reactwg/react-native-new-architecture#135

Reproducible sample code -

import React, { Component } from 'react';
import {
AppRegistry,
StyleSheet,
View,
} from 'react-native';

import { SketchCanvas } from '@terrylinla/react-native-sketch-canvas';

export default class example extends Component {
  render() {
    return (
      <View style={styles.container}>
        <View style={{ flex: 1, flexDirection: 'row' }}>
          <SketchCanvas
            style={{ flex: 1 }}
            strokeColor={'red'}
            strokeWidth={7}
          />
        </View>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1, justifyContent: 'center', alignItems: 'center', backgroundColor: '#F5FCFF',
  },
});

AppRegistry.registerComponent('example', () => example);

Steps to reproduce

• yarn add react-native-sketch-canvas
• cd ios
• bundle install
• RCT_NEW_ARCH_ENABLED=1 bundle exec pod install
• cd ..
• open android/gradle.properties
• change the newArchEnabled boolean from false to true
• Add the component to unstable_reactLegacyComponentNames array in react-native.config.js
• npx react-native run-ios
• npx react-native run-android
• Update the App.js file with the snippet in the Reproducible Sample Code section

Expected result -

• Module opens and we are able to save the edited sketch

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

[ravindraguptacapgemini]

@cipolleschi can you help here?

[cipolleschi]

@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

[karthiktrcapgemini]

Sure @cipolleschi, We will share a reproducible repo as soon as possible.

[karthiktrcapgemini]

@cipolleschi - Here is the reproducible repo for this -> https://github.com/karthiktrcapgemini/react-native-sketch-canvas-interop-layer-issue

Adding a few more details -

Android -

• Without adding the component to unstable_reactLegacyComponentNames - Result - Standard "RNSketchCanvas is not fabric compatible yet" red screen is shown
• After adding the component to unstable_reactLegacyComponentNames - Result - The feature opens but we are unable to sketch, there is no error or exception shown. Nothing happens, its a blank screen with the given background colour to sketch canvas.

iOS -

• Without adding the component to unstable_reactLegacyComponentNames - Result - Standard "RNSketchCanvas is not fabric compatible yet" red screen is shown
• After adding the component to unstable_reactLegacyComponentNames - Result - We are able to sketch on the canvas but it seems the mechanism to fetch the base64 png image of the sketch is failing at the native code. Please see SS

[cipolleschi]

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?

[karthiktrcapgemini]

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

[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.

[karthiktrcapgemini]

Thanks a lot @cipolleschi, this sounds great. I will keep this thread posted once I get a chance to test this fix for iOS

[cipolleschi]

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.

[karthiktrcapgemini]

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.

[ravindraguptacapgemini]

@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.

[cortinico]

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:

react-native-sketch-canvas/android/src/main/java/com/terrylinla/rnsketchcanvas/SketchCanvasManager.java

Line 93 in b322a86

public void receiveCommand(SketchCanvas view, int commandType, @Nullable ReadableArray args) {

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: facebook/react-native@c7aaae6 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.

[karthiktrcapgemini]

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?

[cortinico]

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

[karthiktrcapgemini]

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 -

React Native Version

0.72.4

What platforms are you seeing the problem on?

Android

Device(s)

Android Pixel 5 (API 33)

Issue

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.

iOS

Android

Reproducer APP - I have updated the reproducer app to log the base64 result from the method

https://github.com/karthiktrcapgemini/react-native-sketch-canvas-interop-layer-issue

[cortinico]

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:

react-native-sketch-canvas/src/SketchCanvas.js

Lines 147 to 153 in b322a86

	getBase64(imageType, transparent, includeImage, includeText, cropToImageSize, callback) {
	if (Platform.OS === 'ios') {
	SketchCanvasManager.transferToBase64(this._handle, imageType, transparent, includeImage, includeText, cropToImageSize, callback)
	} else {
	NativeModules.SketchCanvasModule.transferToBase64(this._handle, imageType, transparent, includeImage, includeText, cropToImageSize, callback)
	}
	}

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.

[iBotPeaches]

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