[FEAT] TurboModules guide by lindboe · Pull Request #3168 · react/react-native-website

lindboe · 2022-06-20T22:21:46Z

This PR is a draft of the TurboModules guide, based on the existing work for the Fabric Components guide. The code here is pulled from https://github.com/react-native-community/RNNewArchitectureLibraries/tree/feat/back-turbomodule as much as possible.

Testing Status

These steps worked for me on Android, but iOS is failing to load the TurboModule. No build errors, and the generated code looks fine as far as I can tell. I tried renaming the module to Calculator as in the original example but still no luck.

I have the module code and app I used here: https://github.com/lindboe/TMGuide

Still need to fix prior to merge (usually marked by TODO)

Troubleshooting section: I'd like to add the following sections, but I'm not sure what instructions to write because I get stuck on it myself:
- How to troubleshoot a native module that won't load
- Managing the Android build process if you've made a mistake, went to correct it, and ended up with mismatched native/JS specs, which produces an error

Comparison with Fabric Component guide

There's a lot of overlap of the ground we want to cover between this guide and the TurboModule guide. I think there may even be an argument for extracting some of it into a separate, shared document (primarily around understanding JavaScript concepts).

I think it's worth going through and comparing the two and picking the preferred versions of the shared text.

These are the major themes of what I changed:

Reworded to avoid "we" as recommended in the style guide. Preferred "you", and imperative-style sentences for instructions.
Bulleting:
- Use numbered steps when describing items that happen in order
- Either use capitalization and punctuation in list items (when they are full sentences) or use neither (when they are sentence fragments)
Consistently used tree layout for describing folder structure, instead of image
Added titles to all code blocks (helps re-orient the reader in a long document)
fixed some errors (grammar, mixed use of rnt prefix)
fix for tab values (the Fabric guide version was crashing)

Other notes

Codegen vs CodeGen?

I'm seeing both cases around, which is preferred?

JS best practices

On the Fabric guide PRs, we talked about wanting to explain JS concepts for devs who may not be familiar. What does this mean for whether we want to demonstrate JS best practices? For example, we explain imports/exports, but don't follow the best practice of a module using an index file to properly export/import its API.

Document structure

The document gets quite long, and interleaved iOS/Android instructions might make it easier to lose your place. I wonder if it might be beneficial to split the instructions by platform? Obviously then we'd still have some repetition in the JS configuration.

Explaining CodeGen

I followed the Fabric guide's lead on this, but I noticed that we have phrases like "For further information on how the CodeGen, have a look here", fairly late in the document after we've already mentioned CodeGen several times. I think we might want to refactor that structure.

lindboe · 2022-06-20T22:24:03Z

+  add(a: number, b: number): Promise<number>;
+}
+
+export default (TurboModuleRegistry.get<Spec>(


Just a point of curiosity: Does exporting default like this defeat lazy loading? Is lazy loading obtained a different way? Or is that lazy loading feature entirely on the native side? (I assume not or I'd expect TurboModuleRegistry.get to return a promise)

Does exporting default like this defeat lazy loading?

Not necessarily. The TM will be lazy loaded as soon as you call TurboModuleRegistry.get. So they will be loaded whenever you enter in a code flow that imports this spec file.

From prior experience with things like this, my understanding is that get would be called as soon as this was imported in another file, so in the example app it's not being lazily loaded. We'd either have to export a function that calls get or conditionally require this export to have lazy loading, correct?

cortinico · 2022-06-22T18:54:50Z


-This section is a step-by-step guide to create a TurboModule from scratch. The list of subsections is roughly:
+- Strongly typed interfaces that are consistent across platforms
+- The ability to create shared C++ code for use across platforms


Suggested change

- The ability to create shared C++ code for use across platforms

- The ability to create C++ only module to use across platforms

I've added 'only' here. Feel free to reword as you prefer @lindboe. I would just mention that this allows to reduce the code to maintain.

@cortinico can you elaborate for me more what you want to convey by including "C++-only" here? Is there a misunderstanding you're trying to avoid? To my eye, I wouldn't assume the original wording means that I can share code in other languages as well.

(Side note: it would have to be written "C++-only", which looks a bit odd with the "++-", so I would try to reword this once I have a better understanding of intent.

I think we are saying two different things:

In the original formulation, we wanted to convey that we can share code between iOS and Android using C++ code.

In the suggestion by Nico, we are suggesting that we can create TM that can be used on both platforms and that contains only C++ code (this can be used for TM that do not need to access specific platform's features).

Both statements are true. I would specify both or look for a formulation that implies both meanings. What do you think?

Tried to rephrase this to the best of my ability to explain both those ideas (thanks for laying them out so clearly @cipolleschi). Let me know what you think!

Seems great, thank you!

cortinico · 2022-06-22T19:05:09Z

+To create a TurboModule, we need to:
+
+1. Define a set of JavaScript specifications.
+2. Configure the module and inspect the code created by Codegen.


Suggested change

2. Configure the module and inspect the code created by Codegen.

2. Configure the module and let the Codegen generate the scaffolding.

scaffolding can be replaced with boilerplate or other similar terms.

I would just not mention 'inspect the code' in the list of necessary steps.

I like scaffolding, I think that's very clear about the role it plays, and implies you're expected to fill in more code.

I'm guessing the original intent in the "inspect the code" wording (from the Fabric Guide) was to talk about how we're going to read the scaffolding generated so we understand what native code to write, which does seem like a useful part of the process to convey, to me?

Also just a note, but a few different suggestions left mention using "the CodeGen". Just to make sure we're clear, I don't think this is grammatically correct, it should either be "let CodeGen generate" or "let the CodeGen CLI tool (or process, or script, etc) generate..."

Addressed, but leaving open for discussion if needed

I'm guessing the original intent in the "inspect the code" wording (from the Fabric Guide) was to talk about how we're going to read the scaffolding generated so we understand what native code to write, which does seem like a useful part of the process to convey, to me?

That's correct, that was my intent there. The user will need to read the code generated by CodeGen in order to know how to use the generated types and files.

In this bullet list, I think that both the sentences fit. I will use the one that requires less effort from the reader (I think it is Nico's suggestion)

cortinico · 2022-06-22T19:07:16Z

+
+export default (TurboModuleRegistry.get<Spec>(
+  'RTNCalculator'
+) as Spec | null);


Why is this | null needed?

This is my understanding of the return value of TurboModuleRegistry.get can be: either an object that implements Spec or null. Is that incorrect?

Having:

export default (TurboModuleRegistry.get<Spec>( 'RTNCalculator' ): ?Spec);

should be sufficient

I might not be understanding: this is the TypeScript example, but that looks like Flow syntax?

I agree with Lizzi here. For TypeScript examples is better to use TypeScript idiomatic syntax.

cortinico

Thanks for sendign this over @lindboe. Sorry it took me a bit longer than usual to review this. The content looks good on my end 👍 Left several comments which needs to be addressed, but nothing that requires major work or restructuring.

cortinico · 2022-06-22T20:24:53Z

+  add(a: number, b: number): Promise<number>;
+}
+
+export default (TurboModuleRegistry.get<Spec>(


Does exporting default like this defeat lazy loading?

Not necessarily. The TM will be lazy loaded as soon as you call TurboModuleRegistry.get. So they will be loaded whenever you enter in a code flow that imports this spec file.

cortinico · 2022-06-22T21:12:13Z

+There are other macros that can be used to export modules and methods. You view the code that specifies them [here](https://github.com/facebook/react-native/blob/main/React/Base/RCTBridgeModule.h).
+:::
+
+TODO: more description?


Asking @cipolleschi here for a feedback

I would add a small paragraph on the getTurbomodule: method:

Finally, the `getTurboModule:` is used by the New Architecture to create a proper instance of the TurboModule so that the JavaScript side can properly invoke its methods. The function is defined in (and requested by) the `RTNCalculatorSpec.h` file that is generated by CodeGen.

Or something similar.

cortinico · 2022-06-23T22:42:37Z

Answering inline to your questions on the PR description:

These steps worked for me on Android, but iOS is failing to load the TurboModule. No build errors, and the generated code looks fine as far as I can tell. I tried renaming the module to Calculator as in the original example but still no luck.

I've looked at your code on lindboe/TMGuide and it looks everything fine on my end. Perhaps @cipolleschi can spot what's the problem here. I suspect there might be something going on the app setup.

There's an ordering issue with the steps for Codegen with Android here (and I suspect with Fabric Components as well): trying to run Codegen without the Android source files present doesn't work. But we're presenting the Codegen step as how one would know what native code to write, so I'm not sure how to fix that problem.

Discussed offline.

Troubleshooting section: I'd like to add the following sections, but I'm not sure what
instructions to write because I get stuck on it myself:
How to troubleshoot a native module that won't load

This would be valuable, but I think it could be helpful to write it from the 'error message' point of view. Like, "How to resolve when you get the error message xyz".

Managing the Android build process if you've made a mistake, went to correct it, and ended up with mismatched native/JS specs, which produces an error

Same as above.

Comparison with Fabric Component guide There's a lot of overlap of the ground we want to cover between this guide and the TurboModule guide. I think there may even be an argument for extracting some of it into a separate, shared document (primarily around understanding JavaScript concepts).

Agree here. In general they were split originally as we wanted to offer separate guide for TM users vs Fabric users (as the initial playbook was already extremely long).

These are the major themes of what I changed

Thanks for doing all of them 🙏

Codegen vs CodeGen?

IMHO CodeGen should be preferred as it's essentially the name of a product (like Fabric or TurboModule).

JS best practices. [...] For example, we explain imports/exports, but don't follow the best practice of a module using an index file to properly export/import its API.

In theory I agree here. I'm also not a JS expert so I can't make a call here, but I could say that I would try to stay focused on the React Native side of things. We don't need to re-explain JavaScript essentially (i.e. no need to explain what export does, unless is critical for the sake of the guide). I'm fine with having a couple of sentences for the imports and essentially walk the users through the code.

The document gets quite long, and interleaved iOS/Android instructions might make it easier to lose your place. I wonder if it might be beneficial to split the instructions by platform? Obviously then we'd still have some repetition in the JS configuration.

Our hope is to try to align the 2 platforms as much as possible. E.g. in 0.70 the configuration of the two platforms will be aligned. Keeping this in mind, I would try to not split it

Explaining CodeGen
I followed the Fabric guide's lead on this, but I noticed that we have phrases like "For further information on how the CodeGen, have a look here", fairly late in the document after we've already mentioned CodeGen several times. I think we might want to refactor that structure.

Yup that page is going to be a deep dive around Codegen. I think we can move the link a bit earlier just so the reader knows where to find further docs if they need to.

lindboe · 2022-06-25T01:22:00Z

How to troubleshoot a native module that won't load

This would be valuable, but I think it could be helpful to write it from the 'error message' point of view. Like, "How to resolve when you get the error message xyz".

For the TurboModule that won't load (like I was encountering with iOS here),there is no error message, it just returns null, and I'm rather stuck on it so I'm not sure I know what to write as advice! 😄

For the Android codegen failure, shouldn't be too hard to get that error message again to use.

IMHO CodeGen should be preferred as it's essentially the name of a product (like Fabric or TurboModule).

Happy to update it all to this, but as I was going through and doing that, I noticed that the actual code pieces that refer to CodeGen do it as "Codegen":

./gradlew generateCodegenArtifactsFromSchema

s.dependency "React-Codegen"

It might be more confusing to use "CodeGen" in prose if the code consistently uses "Codegen". (For what it's worth I don't think either looks wrong, it's a stylized name so either is acceptable.)

On the overlap of this guide and Fabric:

there are several items that are now inconsistent, or giving feedback on over here; it might be best to go through and do another pass over both the guides to get the guides consistent with each other. This would help with the questions around structure, grammar, how much to address JS, etc.

lindboe · 2022-06-25T01:22:24Z

I pushed an update with the smaller fixes requested. I have not tested the code changes yet, but they seemed relatively safe.

cipolleschi · 2022-06-27T11:55:40Z

Answering inline to your questions on the PR description [cit]:

These steps worked for me on Android, but iOS is failing to load the TurboModule. No build errors, and the generated code looks fine as far as I can tell. I tried renaming the module to Calculator as in the original example but still no luck.

I will look into it after lunch. Hopefully it is just something misconfigured.

It might be more confusing to use "CodeGen" in prose if the code consistently uses "Codegen". (For what it's worth I don't think either looks wrong, it's a stylized name so either is acceptable.)

That's a good point... I'd say that we can revert it to Codegen everywhere for consistency, but I'll do that after we merge everything so that we just have to run the rename only once (I'll create a task for that)
@cortinico what do you think?

there are several items that are now inconsistent, or giving feedback on over here; it might be best to go through and do another pass over both the guides to get the guides consistent with each other. This would help with the questions around structure, grammar, how much to address JS, etc.

I will update the Fabric Guide this afternoon or tomorrow, trying to align the two.

cipolleschi

Great guide! Thanks for writing it. I'll now look at the repo and see why it is not working!

cipolleschi · 2022-06-27T11:58:33Z

+
+In order to keep the module decoupled from the app, it's a good idea to define the module separately from the app, and then add it as a dependency to your app later. This is also what you'll do for writing TurboModules that can be released as open-source libraries later.
+
+Next to your application, create a folder called `RTNCalculator`. (**RTN** stands for "**R**eact**T** **N**ative", and is a standard prefix for React Native modules).


Suggested change

Next to your application, create a folder called `RTNCalculator`. (**RTN** stands for "**R**eact**T** **N**ative", and is a standard prefix for React Native modules).

Next to your application, create a folder called `RTNCalculator`. (**RTN** stands for "**R**eac**T** **N**ative", and is a conventionally chosen prefix for React Native modules).

For context, it is not really a standard, there is no standard here. The creator of a Library can choose the prefix it wants. Typically companies chooses a prefix that comes from their company name, here we work on React Native and we chosen that prefix. I'm not convinced about the conventionally chosen wording, though.

I think "standard" is also okay to mean what you're conveying here ("a standard" can mean "a required method of doing thing", but it can also mean "conventional"). But "conventional prefix" is also okay, but it sounds more awkward to me.

Addressed, chose "recommended" as I think it conveys the idea better

cipolleschi · 2022-06-27T12:16:04Z

+
+The generated code is stored in the `MyApp/node_modules/rtn-calculator/android/build/generated/source/codegen` folder and it has this structure:
+
+TODO: ordering issue. I can't actually get codegen to work at this point. Only works once I add the native files following. Probably similar issue for Fabric guide. Including generated file structure for later ease of use:


Do you have errors here or the CodeGen just does not generate anything?

Just generates nothing. Discussed in Discord.

We went further in the exploration and you were right. Android requires other files in order to be picked up by the Codegen. Specifically, it needs also:

The Android Manifest

A file that implements the ReactPackage interface or that extends the TurboReactPackage class

From the conversation in Discord, I'm not sure still exactly how much code I would write for that TurboReactPackage class before running Codegen; especially since I know I've run into issues on the Android side with creating an incomplete or incorrect interface before and then changing it.

cipolleschi · 2022-06-27T14:27:38Z

@lindboe The problem with iOS was autolinking. Versions of React Native < 0.69 use a version 7.x of the React Native CLI which requires an xcodeproj in order to autolink the pod. This requirement has been dropped in React Native CLI 8.x, which is used by React Native 0.69.

I tried to create AnApp at the same level of MyApp using version 0.69 and the pod install step reports:

...
Auto-linking React Native module for target `AnApp`: rtn-calculator
Setting USE_CODEGEN_DISCOVERY=1
[Codegen] Adding script_phases to React-Codegen.
[Codegen] Generating ./build/generated/ios/React-C
...

and

...
Installing hermes-engine (0.69.0)
Installing libevent (2.1.12)
Installing rtn-calculator (0.0.1)
Generating Pods project
...

And the app works. Given that we already released 0.69, and that nothing in the guide has an explicit reference to 0.68, we should be good to go.

lindboe · 2022-06-27T16:33:48Z

Given that we already released 0.69, and that nothing in the guide has an explicit reference to 0.68, we should be good to go.

Should we at least address that this is targeted at a minimum version of 0.69? (I know that the plan is to update to current versions as we go, but thinking about it, people may have a reason to not be on latest RN but still want to make a TurboModule, so having versioned docs does seem helpful).

cortinico · 2022-06-27T19:12:01Z

That's a good point... I'd say that we can revert it to Codegen everywhere for consistency, but I'll do that after we merge everything so that we just have to run the rename only once (I'll create a task for that)
@cortinico what do you think?

I don't have a strong opinion at this point 🤔 I'm fine picking one of the two. We can do a pass afterwards and fix everything. Nit: we also have case-police set up in the repo. Perhaps we can configure a custom dictionary and add CodeGen, TurboModules and all the other cases we want to enforce (ref here: https://github.com/antfu/case-police).

cortinico

Did a final pass and this is great on my end 👍 I'll leave this to @cipolleschi to merge it whenever you wish. Thanks again for helping us with this @lindboe 🙏

cortinico · 2022-07-20T10:56:21Z

+cd MyApp
+yarn add ../RTNCalculator
+cd android
+./gradlew generateCodegenArtifactsFromSchema --rerun-tasks


Suggested change

./gradlew generateCodegenArtifactsFromSchema --rerun-tasks

./gradlew generateCodegenArtifactsFromSchema

@cipolleschi Similarly for the Fabric guide, I believe we should not suggest to --rerun-tasks.

…turbomodules