[@unimodules/react-native-adapter] Use Proxy to throw UnavailabilityErrors

[sjchmiela]

Why

We tend to have our unimodules' code littered with

if (!NativeModule.someMethodAsync) {
  throw new UnavailabilityError('NativeModule', 'someMethodAsync');
}
return await NativeModule.someMethodAsync();

I wanted to write the proxy like this some time ago, but the Android's JSC version didn't let us use Proxy. Now, as JSC is up-to-date, we can.

Searching for throw new UnavailabilityError results in 224 results in 56 files. Most of it is as simple as the example snippet above, some of them do parse arguments in some way. In my opinion going from

export async function manipulateAsync(/* args */): Promise<ImageResult> {
  if (!ExpoImageManipulator.manipulateAsync) {
    throw new UnavailabilityError('ImageManipulator', 'manipulateAsync');
  }
  if (!(typeof uri === 'string')) {
    throw new TypeError('The "uri" argument must be a string');
  }
  return await ExpoImageManipulator.manipulateAsync(uri, actions, { format, ...rest });
}

to

export async function manipulateAsync(/* args */): Promise<ImageResult> {
  if (!(typeof uri === 'string')) {
    throw new TypeError('The "uri" argument must be a string');
  }
  return await ExpoImageManipulator.manipulateAsync(uri, actions, { format, ...rest });
}

(i.e. changing the order of errors thrown in case uri isn't a string) shouldn't be a big change.

Disadvantages

On the other hand, throwing errors on get of a property will result in a breaking change of behavior. For example the following code will no longer be correct:

let manifest = null;
if (ExponentConstants && ExponentConstants.manifest) {
  manifest = ExponentConstants.manifest;
// ...

it would need to be rewritten to

let manifest = null;
if (ExponentConstants && 'manifest' in ExponentConstants) {
  manifest = ExponentConstants.manifest;
// ...

Same would go for

const { name, ...constants } = (ExponentConstants || {}) as any;

which would need to be

const { name, ...constants} = ({ ...(ExponentConstants || {}) }) as any;

Another way of solving this constants issue would be to introduce the getConstants method through which constants will be accessed in TurboModules environment. It would return a plain object so there wouldn't be any problem with executing ExponentConstants.getConstants().manifest if ExponentConstants wouldn't expose a manifest constant.

How

Instead of constructing a big "proxy" object when starting up the application we can use Proxy to create the modulesProxies on demand.

Test Plan

Calling await Application.getInstallReferrerAsync() in a for loop 5000 times in bare-expo project took:

• 78.6 s, 76.5 s, 78.3 s, 75.8 s, 78.2 s, 80.0 s, 79.5 s, 82.1 s, 77.8 s — mean 78.5 s

whereas on master the same benchmark took

• 83.2 s, 78.4 s, 78.4 s, 78.3 s, 79.0 s, 77.4 s, 81.5 s, 79.4 s, 79.1 s — mean 79.4 s

It seems that replacing if (Module.method) { … } with Proxy shouldn't affect performance negatively.

[ide]

IIRC Hermes has some issues with Proxies (that might have been fixed, though).

[ide]

The 'property' in object checks make me think this needs more work. It needs to be really obvious when standard JavaScript (object.property) doesn't work, or ideally standard JS would just work everywhere. Generally when libraries become more mature they remove magic and dynamic features and move towards plain, static code that requires less context to understand. Proxies add magic and I think this is one of the (very many) scenarios in which they might not be right.

[sjchmiela]

I agree. I consider this PR more of a "wouldn't it be good if" than a serious change proposal.

Maybe a prerequisite for this kind of change would be introducing the aforementioned getConstants() method to the NativeModulesProxy API. This way we would be able to differentiate module's properties from methods and design a Proxy that would look like:

{
  // getConstants returns a plain object
  getConstants: () => { constantName: "constantValue", ... },

  // existing native methods return what they already are
  existingNativeMethodAsync: () => NativeProxy.callMethod(...),
  anotherExistingNativeMethodAsync: () => NativeProxy.callMethod(...),

  // since constants only belong to getConstants,
  // any other `get` must expect to receive an exported method
  [default]: () => { throw new UnavailabilityError() }
}

Even though this still would have to use Proxy, it wouldn't be such a secret magic after all.

[ide]

Yeah. I think getConstants and Turbomodules helps a lot since all native implementations have to implement the same interface. There are no more “if (Module.method)” checks.

[sjchmiela]

This won't get merged in current state so I'll let myself close this pull request.

Adding to the topic of discussion: I quite like the approach I took with expo-notifications, where to indicate that we need to confirm that the native method is available before we call it we use optional type, like:

expo/packages/expo-notifications/src/PushTokenManager.types.ts

Lines 1 to 5 in db1c413

	import { ProxyNativeModule } from '@unimodules/core';
	export interface PushTokenManagerModule extends ProxyNativeModule {
	getDevicePushTokenAsync?: () => Promise<string>;
	}

if the native module is typed like this, it's obvious and the types enforce it that we need to either check if (method) or force-unwrap the method.