OP-SQLite cannot build with 0.74RC and bridgeless

[ospfranco]

Description

Hi, I'm trying to test the library against the latest RC and also trying to get bridgeless to work. So far I'm facing two issues with the included example app in my library repo.

iOS:

It seems the codegen script is expecting the src files to be in node_modules, but the builder-bob template links the dependency directly via react-native.config.js:

// react-native.config.js
const path = require('path');
const pak = require('../package.json');

module.exports = {
  dependencies: {
    [pak.name]: {
      root: path.join(__dirname, '..'),
    },
  },
};

And I get the following error when trying to install the pods:

Android

After turning on all the flags (and bumping the minSDK to 23) I get a compilation error but no output to what is wrong:

Steps to reproduce

If you want to take a look yourself here is the branch. Just install deps via yarn and then try to run the example app: cd example && yarn android or the equivalent iOS commands.

React Native Version

0.74.0-rc.0

Affected Platforms

Build - MacOS

Areas

Bridgeless - The New Initialization Flow

Output of npx react-native info

System:
  OS: macOS 14.3.1
  CPU: (11) arm64 Apple M3 Pro
  Memory: 2.49 GB / 36.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.19.0
    path: ~/.nvm/versions/node/v18.19.0/bin/node
  Yarn:
    version: 4.0.2
    path: ~/.nvm/versions/node/v18.19.0/bin/yarn
  npm:
    version: 10.2.3
    path: ~/.nvm/versions/node/v18.19.0/bin/npm
  Watchman: Not Found
Managers:
  CocoaPods:
    version: 1.14.0
    path: /Users/osp/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.2
      - iOS 17.2
      - macOS 14.2
      - tvOS 17.2
      - visionOS 1.0
      - watchOS 10.2
  Android SDK:
    Android NDK: 26.0.10792818
IDEs:
  Android Studio: 2023.1 AI-231.9392.1.2311.11330709
  Xcode:
    version: 15.2/15C500b
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.10
    path: /usr/bin/javac
  Ruby:
    version: 3.3.0
    path: /Users/osp/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.0-rc.0
    wanted: ^0.74.0-rc.0
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

### Stacktrace or Logs

```text
Pasted above

Reproducer

https://github.com/OP-Engineering/op-sqlite/tree/bridgeless

Screenshots and Videos

No response

[github-actions]

⚠️	Missing Reproducible Example
ℹ️	We could not detect a reproducible example in your issue report. Please provide either: If your bug is UI related: a Snack If your bug is build/update related: use our Reproducer Template. A reproducer needs to be in a GitHub repository under your username.

[cipolleschi]

Hi @ospfranco, thanks for the issue.

For Android, we are aware that codegen and autolinking are not working with the RC.0 as we needed a commit that landed right after the branch cut. It should be solved in RC1

For iOS, @dmytrorykun, can you provide any insight? I remember that we had a similar report of react-native.config.js not being picked up by Codegen, but I remember we fixed that already. 🤔

[ospfranco]

For Android, I'm not sure this is the error I'm seeing? seems it doesn't like that I use the CallInvokerHolderImpl reference and it fails the compilation? How am I supposed to get a hold of the CallInvoker?

The class

package com.op.sqlite

import com.facebook.react.bridge.ReactContext
import com.facebook.react.turbomodule.core.CallInvokerHolderImpl

class OPSQLiteBridge {
    private external fun installNativeJsi(
        jsContextNativePointer: Long,
        jsCallInvokerHolder: CallInvokerHolderImpl,
        docPath: String
    )

    private external fun clearStateNativeJsi()
    fun install(context: ReactContext) {
        val jsContextPointer = context.javaScriptContextHolder!!.get()
        val jsCallInvokerHolder =
            context.catalystInstance.jsCallInvokerHolder as CallInvokerHolderImpl
        // Trick to get the base database path
        val dbPath =
            context.getDatabasePath("defaultDatabase").absolutePath.replace("defaultDatabase", "")
        installNativeJsi(
            jsContextPointer,
            jsCallInvokerHolder,
            dbPath
        )
    }

    fun clearState() {
        clearStateNativeJsi()
    }

    companion object {
        val instance = OPSQLiteBridge()
    }
}

The error logs:

e: file:///Users/osp/Developer/op-sqlite/android/src/main/java/com/op/sqlite/OPSQLiteBridge.kt:9:30 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: file:///Users/osp/Developer/op-sqlite/android/src/main/java/com/op/sqlite/OPSQLiteBridge.kt:17:61 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: file:///Users/osp/Developer/op-sqlite/android/src/main/java/com/op/sqlite/OPSQLiteBridge.kt:21:9 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: file:///Users/osp/Developer/op-sqlite/android/src/main/java/com/op/sqlite/OPSQLiteBridge.kt:23:13 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.

[cortinico]

For Android, I'm not sure this is the error I'm seeing? seems it doesn't like that I use the CallInvokerHolderImpl reference and it fails the compilation? How am I supposed to get a hold of the CallInvoker?

You're seeing this error because you're using CallInvokerHolderImpl which is annotated as FrameworksAPI. This is a more advanced API that should be used only if you know what you're doing.

1. Can I ask you why do you need to access CallInvokerHolderImpl.
2. What's your use case?
3. What does the installNativeJsi method does?

For the time being, you can overcome this build failure by annotating your class with:

import com.facebook.react.common.annotations.FrameworkAPI

@OptIn(FrameworkAPI::class)
class OPSQLiteBridge {
  ...
}

[ospfranco]

The methods on op-sqlite run in a different thread, therefore to return the data to JS without crashing the JS context I need to schedule the callback (or in this case promise resolve). I have used this extensively on other client modules whenever there needs to be async work that does not block the JS thread, e.g. authentication/fingerprint callbacks.

Edit: I read up a bit on the RuntimeExecutor, which I guess is meant to replace the CallInvoker, I don't understand all the details though. Any pointers would be appreciated to understand where is the new arch headed to.

[ospfranco]

After adding the annotation I'm getting the following exception when trying to access the callinvokerholderimpl:

2024-02-28 08:24:19.849  2370-2475  OPSQLite                com.op.sqlite.example                E  Install exception: java.lang.UnsupportedOperationException: There is no Catalyst instance in bridgeless mode.

What would be the correct way to access the callinvoker on bridgeless?

[ospfranco]

Trying iOS again, and added a console.log on the paths being expanded, indeed it seems it looks in node_modules and then in a prepended path

I guess this is correct on a normal installation but on a library setup where the dependency is not explicitly declared on the package.json but instead linked via react-native.config.js and via tsconfig paths... it will never find the source

[dmytrorykun]

@ospfranco sorry, I forgot to post an update here. iOS has been fixed #43210
The fix will be available in the next RC.

[ospfranco]

Thanks in the mean time I will patch it

[ospfranco]

Now getting this error. Any ideas?

[dmytrorykun]

@ospfranco pod install?

[ospfranco]

Tried a nuke of pods and caches and still getting the same error.

Getting this error because of importing the RCTBridge+Private header apparently? But without it I don't have access to the runtime and callInvoker types.

Edit: ah right bridgeless... so what's the correct way to get access to the jsiRuntime?

[ospfranco]

@philIip I just tried to patch with your changes and I'm getting this error(s):

I think my changes are correct, so don't know why am I getting that missing header error

I changed both node_modules in the example project and parent folder

[arushikesarwani94]

@ospfranco I was able to repro the insta crash of OPSQLiteExample app locally just not with the same error as yours and rather as a SEG Fault as discussed in chat.

Both the BridgelessCatalystInstance & CallInvokerHolder are getting retrieved correctly upon debugging without needing any cast.

I was able to get past the app crash by updating the cpp-adapter.cpp to exactly the one on main

Since you mentioned the change was not necessary for bridgeless as long as both the runtime and the invoker are there is all good, I think this should unblock you.

[ospfranco]

Thanks for the help @arushikesarwani94! The app is now building correctly! 🎉

Waiting for instructions regarding iOS

[philIip]

Thanks for the help @arushikesarwani94! The app is now building correctly! 🎉

Waiting for instructions regarding iOS

@ospfranco, can you patch #43600? that should resolve your build error - it works when i tested it locally. let me know if there's any unexpected runtime issues.

i will also be picking into RC6 if you're willing to wait!

[ospfranco]

I re-applied the same changes again and I'm getting this:

[ospfranco]

I tried generating a patch-package with the changes but it failed to apply. Maybe some of these files are created on the install step? In any case, manually applying the changes to node_modules/react-native doesn't seem to work for me

Here is the patch file in case you want to check if I did it correctly:

https://github.com/OP-Engineering/op-sqlite/pull/60/files#diff-0c24a4c1e88df14537749bb451dbc3164819872c11700d39fa16f1abe54b1fdc

[philIip]

yea, we probably need a pod install here since there are native changes.

can you show me the stack trace that's pulling in RCTBridge+Inspector? you should be able to find it in the builds tab here, which should open the build logs:

[ospfranco]

Error changed a bit, for some reason my patch is working now:

This is all I can find:

[philIip]

oooo got it, seems like a classic merge conflict lol.... PageTarget got renamed to HostTarget after 0.74 cut. i have to push a change directly to 0.74 that renames HostTarget back to PageTarget in RCTBridge (Inspector) :P

if you rename HostTarget to PageTarget in RCTBridge+Inspector your build should pass now

[ospfranco]

It seems now the error on node modules is gone, but still getting missing property when casting to CxxBridge

[philIip]

do you have the new architecture flag turned on? maybe you're missing a RCT_NEW_ARCH_ENABLED=1 bundle exec pod install IIRC in your code you have the import to RCTBridge+Private gated by a compiler flag.

that being said, you can remove those flags regardless of new arch / old arch. it's backwards compatible, and the symbols that are missing are located there.

[ospfranco]

You are absolutely right, sorry, hard to keep track as I'm busy with paid work. Uncommenting it fixes it in the old arch, but I'm getting a undefined module on the new arch. Checking it now.

[ospfranco]

No type error, but on runtime the cxxbridge is not there

[philIip]

You are absolutely right, sorry, hard to keep track as I'm busy with paid work. Uncommenting it fixes it in the old arch, but I'm getting a undefined module on the new arch. Checking it now.

no need to apologize! just figuring out one thing at a time

No type error, but on runtime the cxxbridge is not there

cool i was just able to repro! :D let me get back to you!

[RSNara]

@ospfranco, I can explain why the cxxBridge is nil here!

Here's the goal: When people migrate to our stable apis (for turbomodules and fabric at least), we want them to cut ties with the backwards compatibility layers (i.e: this bridge proxy). That way, people only have to migrate once for the new architecture (i.e: when they adopt the stable apis). And after most people migrate migrate, as the framework authors, we can just sunset the backwards compatibility layers.

The solution here would be this: just undo the turbomodule migration work you did for this module that accesses the bridge. That way, it'll just become a legacy module going through the interop layer, and it'll have access to the bridge. Once we figure out the stable api for give modules access to the runtime (likely c++ turbomodule), you'll just have to re-write this completely. I hope that's fine!

[ospfranco]

I will give it a try, but this is not a normal turbo module. I just need to access the runtime so I can later bind c++ functions directly

[ospfranco]

@RSNara i also have to say: i won't migrate this module to c++ turbo module, because I need access to native apis, in this case getting the library folder path and on Android getting the databases path.

Same with other libraries I have, sometimes I call native apis from c++

Would this still work with the approach you suggested?

[philIip]

i think to simplify this discussion, essentially if we revert the turbomodule migration, you will be able to access the runtime off of the bridge in bridgeless mode. i totally acknowledge this is confusing, but this is the original design decision made a while ago to couple turbomodule migration with bridgeless migration. if this is too involved, please let us know! we can reconsider this.

when it comes to the stable api (c++ turbomodule), we can tackle that conversation later.

[huntie]

We've picked the requested change (reactwg/react-native-releases#183) into the 0.74-stable branch this morning. It will be part of 0.74 RC6, due out later today.

[ospfranco]

@philIip ok, I got it, it is cumbersome and I'm short on time, but I will try to get it soon a report how it goes. Thanks!

[ospfranco]

Ok, reverting to an old module works in all archs. Will close for now, and I guess we can have a discussion when old modules will be completely retired. Like I said, I don't really care about having to migrate to a C++ Turbo Module, as long as I get to call the native functions that's all I need.