ionic-team / capacitor

Build cross-platform Native Progressive Web Apps for iOS, Android, and the Web ⚡️
https://capacitorjs.com
MIT License
12.11k stars 1k forks source link

feat: type-safe environment-dependent plugins #4096

Open buschtoens opened 3 years ago

buschtoens commented 3 years ago

Feature Request

Description

Right now plugins make themselves available by augmenting the PluginRegistry interface in the @capacitor/core module, for instance like this:

// definitions.d.ts

declare module '@capacitor/core' {
  interface PluginRegistry {
    Foo: FooPlugin;
  }
}

export interface FooPlugin {
  getFoo(): Promise<'foo'>;
  getBar(): Promise<'bar'>;
  getCurrentPlatform(): Promise<'android' | 'ios' | 'web'>;
}

The plugin is included in the registry unconditionally. When a plugin provides uniform implementations for all three supported platforms, this is not an issue.

However, there are plugins which are not available on all platforms or have varying interfaces per platform. For instance, the above could be expressed in a different way, if the registries for each platforms were disjunct.

// definitions.d.ts

declare module '@capacitor/core' {
  interface PluginRegistryAndroid {
    Foo: FooPluginAndroid;
  }
  interface PluginRegistryIOS {
    Foo: FooPluginIOS
  }
  interface PluginRegistryWeb {
    Foo: FooPluginWeb;
  }
}

export interface FooPlugin {
  getFoo(): Promise<'foo'>;
  getBar(): Promise<'bar'>;
  getCurrentPlatform(): Promise<'android' | 'ios' | 'web'>;
}

export interface FooPluginAndroid extends FooPlugin {
  getCurrentPlatform(): Promise<'android'>;
}

export interface FooPluginIOS extends FooPlugin {
  getCurrentPlatform(): Promise<'ios'>;
}

export interface FooPluginWeb extends FooPlugin {
  getCurrentPlatform(): Promise<'web'>;
}

Through some nifty type for Capacitor.Plugins we could then achieve a stricter, safer usage, like this:

import { Plugins } from "@capacitor/core";

declare module "@capacitor/core" {
  interface PluginRegistry {
    LegacyPlugin: { name: "LegacyPlugin" };
  }

  interface PluginRegistryAndroid {
    AndroidPlugin: { name: "AndroidPlugin" };
    NativePlugin: { name: "NativePlugin"; platform: "android" };
  }

  interface PluginRegistryIOS {
    IOSPlugin: { name: "IOSPlugin" };
    NativePlugin: { name: "NativePlugin"; platform: "ios" };
  }

  interface PluginRegistryWeb {
    WebPlugin: { name: "WebPlugin" };
  }
}

// Plugins registered in the legacy `PluginRegistry` are always optional, as
// there is no way to guarantee that they are available. To make them
// accessible without `?.` the plugin author must migrate them to one or more of
// the new registries
Plugins?.LegacyPlugin;  // undefined | { name: "LegacyPlugin" }

// Without narrowing the type down via one of the `.is*` platform
// discriminators, all plugins are available as optional, meaning you have to
// use `?.` when accessing them.
Plugins?.AndroidPlugin; // undefined | { name: "AndroidPlugin" }
Plugins?.IOSPlugin;     // undefined | { name: "IOSPlugin" }
Plugins?.NativePlugin;  // undefined | { name: "NativePlugin"; platform: "android" | "ios" }
Plugins?.WebPlugin;     // undefined | { name: "WebPlugin" }

if (Plugins.isAndroid) {
  Plugins.isIOS; // false
  Plugins.isWeb; // false

  // Android-only plugin is available.
  Plugins.AndroidPlugin; // { name: "AndroidPlugin" }

  // Common native plugin is available and narrowed down to the Android flavor.
  Plugins.NativePlugin;  // { name: "NativePlugin"; platform: "android" };

  // As before, plugins registered in the legacy `PluginRegistry`, are always
  // optional and you have to use `?.`.
  Plugins.LegacyPlugin;  // undefined | { name: "LegacyPlugin" }

  // Plugins that are specific to a platform are not accessible.
  Plugins.IOSPlugin;     // undefined
  Plug

buschtoens/capacitor-plugin-registry/blob/main/demo.ts

Platform(s)

Android, iOS, Web

Preferred Solution

import type { Merge } from "type-fest";

interface PlatformDiscriminator {
  isAndroid: boolean;
  isIOS: boolean;
  isWeb: boolean;
}

// Original legacy registry remains, so that legacy addons can still register themselves.
interface PluginRegistry {}

interface PluginRegistryAndroid {
  isAndroid: true;
  isIOS: false;
  isWeb: false;
}

interface PluginRegistryIOS {
  isAndroid: false;
  isIOS: true;
  isWeb: false;
}

interface PluginRegistryWeb {
  isAndroid: false;
  isIOS: false;
  isWeb: true;
}

// -------------------- Private helper types ------------------------
// These are not meant to be augmented directly by plugins.
// To prevent this, they may be moved to extra files.

type AllPlugins = Partial<
  Merge<
    PluginRegistry,
    Merge<PluginRegistryAndroid, Merge<PluginRegistryIOS, PluginRegistryWeb>>
  >
> &
  PlatformDiscriminator;

type NoPlugins = Record<
  | keyof PluginRegistryAndroid
  | keyof PluginRegistryIOS
  | keyof PluginRegistryWeb,
  undefined
>;

type CapacitorPluginRegistry =
  | AllPlugins
  | (Partial<PluginRegistry> & Merge<NoPlugins, PluginRegistryAndroid>)
  | (Partial<PluginRegistry> & Merge<NoPlugins, PluginRegistryIOS>)
  | (Partial<PluginRegistry> & Merge<NoPlugins, PluginRegistryWeb>);

// This is `Capacitor.Plugins`
export const Plugins: CapacitorPluginRegistry;

buschtoens/capacitor-plugin-registry/blob/main/index.d.ts

imhoffd commented 3 years ago

The entire "plugin registry" is being thrown away in Capacitor 3. Plugins will now be imported directly, so I'm not sure if any of this is useful, unfortunately.

However, there are plugins which are not available on all platforms or have varying interfaces per platform.

This would seem to defeat the purpose of providing cross-platform APIs. The goal is to limit platform-specific code in client applications. We do that by creating unified APIs. The methods should exist--but they may throw exceptions. That's what the new UNAVAILABLE/UNIMPLEMENTED errors are for: https://capacitorjs.com/docs/v3/plugins/web#error-handling

buschtoens commented 3 years ago

Interesting. I'll read through the v3 docs in detail and check the core plugins source code. Thanks!

buschtoens commented 3 years ago

So I went through the source code of the v2 web.ts plugins as well as the v3 ones.

I think that the current solution of either throwing an unavailable / unimplemented error is sub-optimal. I've also seen plugins where no such error is thrown, but a no-op void Promise.resolve() or Promise.resolve({ ... }) with stubbed out data is being returned.

Unfortunately TypeScript doesn't yet have any proper way to indicate that a method may throw (https://github.com/microsoft/TypeScript/issues/13219). IntelliSense suggesting a method that will definitely throw / no-op / return stub data in a web environment without any indication that this will happen (not even a DocBlock comment) IMO is bad DX and can lead to bugs which could have been avoided.

I would suggest to add type discriminators to such plugins, e.g.:

interface ExamplePluginBase {
  isCapacitor: boolean;
  isWeb: boolean;

  methodAvailableEverywhere(): Promise<void>;

  nativeOnlyMethod?(): Promise<void>;
}

interface ExamplePluginCapacitor extends ExamplePluginBase {
  isCapacitor: true;
  isWeb: false;
  nativeOnlyMethod(): Promise<void>;
}

interface ExamplePluginWeb extends ExamplePluginBase {
  isCapacitor: false;
  isWeb: true;
  nativeOnlyMethod: undefined;
}

export type ExamplePlugin = ExamplePluginCapacitor | ExamplePluginWeb;

The above can be expressed in many different ways and optimized with helper types (e.g. Required<T>) for cases where the Capacitor implementation always offers all methods and the Web implementation does not.

This construct would even allow for discerning Android and iOS implementations.

In user code this then allows convenient access to all methods which are supported everywhere, as well as type-safe access to methods which are only supported on either platform:

import { Example } from 'example-plugin';

Example.methodAvailableEverywhere();

if (Example.isCapacitor) Example.nativeOnlyMethod();
if (Example.isWeb) Example.nativeOnlyMethod(); // 💥 Type Error

I think that my issue is that I fundamentally disagree with:

This would seem to defeat the purpose of providing cross-platform APIs. The goal is to limit platform-specific code in client applications. We do that by creating unified APIs. The methods should exist--but they may throw exceptions.

I totally agree that it makes sense to provide a unified API / abstraction for everything that can be unified. But I think hiding away differences which do exist and cannot be unified / normalized, actually is worse than making the user special-handle these and in the worst case can lead to bugs. Having a type system is an advantage that we're not currently leveraging to its full extent. IMO every mistake that can be avoided through static code analysis with reasonable effort, should be avoided.

If a user really doesn't care, they can also just call the optional methods like so:

Example.nativeOnlyMethod.?(); // potentially `undefined` return value
Example.nativeOnlyMethod!(); // runtime error

What do you think?

buschtoens commented 3 years ago

btw I'm happy to send in PRs for implementing this feature. If you still think, that this is not a desirable feature, I won't be grumpy and just create wrapper packages around the plugins. :)