Skip to content
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

create-react-native-library@latest: Modify turbo-modules generation #131

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

jhugman
Copy link
Owner

@jhugman jhugman commented Oct 17, 2024

Fixes #107

Now working with [email protected]

This adds kotlin detection and uses Kotlin if needed, or stays with Java: this adds two Kotlin files and an alternative build.gradle file.

It also adds more checks to the test-turbo-modules.sh script, tightening up podspec and Objective C files.

Finally we add documentation to getting-started.md and contributing-turbo-modules.md.

@jhugman jhugman requested a review from zzorba October 17, 2024 16:20
@jhugman
Copy link
Owner Author

jhugman commented Oct 17, 2024

@Johennes may also be interested in this PR.

@Johennes
Copy link
Collaborator

Thanks for taking this one. Looks like a lot of work. 💪

I tested this by regenerating @unomed/react-native-matrix-sdk with create-react-native-library@latest. The example app works for me on iOS but not on Android:

/Users/jm/Code/react-native-matrix-sdk/android/src/main/java/com/unomed/reactnativematrixsdk/ReactNativeMatrixSdkModule.java:11: error: cannot find symbol
public class ReactNativeMatrixSdkModule extends NativeReactNativeMatrixSdkSpec {

I do have the NativeReactNativeMatrixSdkSpec class output from RN's codegen though.

$ cat android/generated/java/com/unomed/reactnativematrixsdk/NativeReactNativeMatrixSdkSpec.java

/**
 * This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
 *
 * Do not edit this file as changes may cause incorrect behavior and will be lost
 * once the code is regenerated.
 *
 * @generated by codegen project: GenerateModuleJavaSpec.js
 *
 * @nolint
 */

package com.unomed.reactnativematrixsdk;

import com.facebook.proguard.annotations.DoNotStrip;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactContextBaseJavaModule;
import com.facebook.react.bridge.ReactMethod;
import com.facebook.react.turbomodule.core.interfaces.TurboModule;
import javax.annotation.Nonnull;

public abstract class NativeReactNativeMatrixSdkSpec extends ReactContextBaseJavaModule implements TurboModule {
  ...

I didn't have a lot of time to dig into it tonight sadly.

@jhugman
Copy link
Owner Author

jhugman commented Oct 18, 2024

Hmm. This is annoying.

Could you run this from within your checked out ubrn directory, substituting the variables:

./scripts/test-turbo-modules.sh \
  --ubrn-config $YOUR_UBRN_CONFIG_FILE \
  --app-tsx $YOUR_APP_TSX_FILE \
  --builder-bob-version latest \
  --android \
  --ios \
  --keep-directory-on-exit \
  --force-new-directory \
  --slug @unomed/react-native-matrix-sdk \
  --ios-name ReactNativeMatrixSdk \
  "$DIRECTORY"

I am a bit confused why you're seeing java files in src/main/java/com/unomed/reactnativematrixsdk; I would expect you to see Kotlin.

@Johennes
Copy link
Collaborator

I am a bit confused why you're seeing java files in src/main/java/com/unomed/reactnativematrixsdk; I would expect you to see Kotlin.

Funnily I had the exact same thought overnight and dug into that this morning. What happened is I wanted to rm -r android/build before building the example app because I'm frequently getting build failures when switching between ubrn versions because of some cached build artifacts. However, I fat-fingered and deleted the entire android folder instead. That killed build.gradle which you're using to determine whether to generate Kotlin or Java.

I copied build.gradle over from a project freshly generated with create-react-native-library, reran the ubrn generation et voilà the Android build works. 🎉

So the issue was entirely on my part, apologies. 🤦‍♂️

It does feel slightly circular to read the Kotlin flag from a file that ubrn itself generates. I couldn't think of another way to do it either though and if people don't mess up the create-react-native-library generation like I did, things should be fine.

I also tested ubrn from this branch on the repo that I had generated with the old create-react-native-library version and things worked fine there on both platforms.

Thanks again for making ubrn work with the latest create-react-native-library!

@jhugman
Copy link
Owner Author

jhugman commented Oct 18, 2024

It does feel slightly circular to read the Kotlin flag from a file that ubrn itself generates. I couldn't think of another way to do it either though and if people don't mess up the create-react-native-library generation like I did, things should be fine.

I did consider checking in package.json for the creat-react-nativw-library version but couldn't see when they'd started adding versions. Also version number sniffing felt too much like useragent sniffing.

Thanks so much for re-testing. That is such a relief.

@jhugman
Copy link
Owner Author

jhugman commented Oct 22, 2024

Could you review this please @zzorba

[email protected] (latest)

This adds kotlin detection and uses Kotlin if needed, or stays with Java: this adds two Kotlin files and an alternative `build.gradle` file.

It also adds more checks to the test-turbo-modules.sh script, tightening up podspec and Objective C files.
@jhugman jhugman force-pushed the jhugman/107-builder-bob-latest-ii branch from e79db28 to b878f69 Compare October 22, 2024 14:00
Copy link
Collaborator

@zzorba zzorba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I tested this locally and my android build is still working just fine (and I think it uses kotlin).

@jhugman jhugman merged commit 8dc9981 into main Oct 23, 2024
5 checks passed
@jhugman jhugman deleted the jhugman/107-builder-bob-latest-ii branch October 23, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doesn't integrate well with create-react-native-library > 0.35.1
3 participants