Make `TurboModuleRegistry.get(...)` enforcing by default, add `getOrNull(..)`

[mrousavy]

Summary

The Turbo Module guides often encourage the use of TurboModuleRegistry.get(...) and directly exporting that, however this will be cumbersome to use for any library users since they have to check for null before every call to that Turbo Module.

Example:

• TurboModule exports itself:

  export default TurboModuleRegistry.get<Spec>('ExampleModule')
  

• Library users use it:

  import Module from './ExampleTurboModuleSpec'
  
  // instead of 
  const x = Module.add(5, 4)
  
  // ...they now have to write
  let x = 0
  if (Module != null) x = Module.add(5, 4)
  else // what else?
  

In my opinion, get(...) should return the module and throwing if it could not be loaded, whereas getOrNull(...) (or getOrUndefined(...) or tryGet(...)) can be used when fallback behaviour is okay (e.g. when falling back to a JS-based module for react-native-web.

But that should be explicit behaviour, not implied and overseen, such as in this example: https://github.com/talknagish/react-native-turbo-starter/blob/aaba7d8613a4bb943876718bd31d3bdd84805f5d/src/NativeTurboStarter.ts#L34-L35

This PR changes this behaviour:

Before

TurboModuleRegistry.get<Spec>('ExampleModule') // <-- returns Spec | undefined
TurboModuleRegistry.getEnforcing<Spec>('ExampleModule') // <-- returns Spec, throws if not found

After

TurboModuleRegistry.get<Spec>('ExampleModule') // <-- returns Spec, throws if not found
TurboModuleRegistry.getOrNull<Spec>('ExampleModule') // <-- returns Spec | undefined

Changelog

[General] [Changed] - Breaking change: Make TurboModuleRegistry.get(...) enforcing by default, add getOrNull(...)

Test Plan

A

1. Try to get a Turbo Module that doesn't exist
2. See if get(..) throws and getOrNull(...) doesn't.

B

1. See if RN still starts correctly with all changes updated

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 23b6240b256b2afb0e5efb2cef1ce27cbd0fcdf8 Branch: main

[cipolleschi]

Hi @mrousavy, thanks for the PR. This is a good call, and it can really improve our users codebase.

I only have one concern here: although this makes it easier to write code, we need to write the TM specs in Flow or Typescript. The transpilers should help you remember that the type is optional and to check for it. That's not true when a function throws. I'm wondering whether we are removing a compile-time check, exposing users to the risk of runtime crashes. What do you think?

Also, could you have a look at the tests? I can see 9 pipelines out of 19 failing in CI.

[mrousavy]

Whoops - I accidentally had a double < in there. Fixed it, tests should be passing now!

I think the optional issue you mentioned in your comment is exactly what I'm trying to solve - if the library author expects the native module to be null (e.g. when running in a different environment such as Windows, or Web context, or older RN version) he uses getOrNull. When the user expects the module to be there, he uses .get(...) just like with the legacy bridge architecture (remember requireNativeComponent?)

[github-actions[bot]]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.