palantir / conjure-typescript

Conjure generator for TypeScript clients
Apache License 2.0
17 stars 16 forks source link

RFC: branded types for aliases and external types (for strings or more) #23

Open mradamczyk opened 6 years ago

mradamczyk commented 6 years ago

Currently, even if a named type in Conjure is distinguishable from a string, a typescript declaration will just have string in place of the type. That can easily lead to problems where type A is used instead of type B (which wouldn't happen eg. with conjure generated java).

I propose to generate stronger typed interfaces for named strings (aliases, rids, external objects with base-type string). The behavior might require to "opt-in" so that it won't cause a breaking change.


Example - Alias

ExampleAlias:
  alias: string

could produce:

export type ExampleAlias = string & {
    __conjure_branded: "ExampleAlias";
};
export const ExampleAlias = {
    of: (exampleAlias: string): ExampleAlias => exampleAlias as ExampleAlias,
};

Example - Import

imports:
  ResourceIdentifier:
    base-type: string
    external:
      java: com.palantir.ri.ResourceIdentifier

could produce:

export type ResourceIdentifier = string & {
    __conjure_branded: "ResourceIdentifier";
};
export const ResourceIdentifier = {
    of: (resourceIdentifier: string): ResourceIdentifier => resourceIdentifier as ResourceIdentifier,
};

With such declarations, we can get much stronger typings:

function foo1(bar: ExampleAlias) {
    return bar;
}
foo1("abc"); // ERROR - TS doesn't allow to use an unnamed string where ExampleAlias is expected
foo1("abc" as ExampleAlias); // OK
foo1(ExampleAlias.of("abc")); // OK
foo1(ResourceIdentifier.of("def")); // ERROR - TS doesn't allow to use another branded string

function foo2(bar: string) {
    return bar;
}
foo2("abc"); // OK
foo2("abc" as ExampleAlias); // OK
foo2(ExampleAlias.of("abc")); // OK - as we don't expect ExampleAlias excplicitly
foo2(ResourceIdentifier.of("def")); // OK - as we don't expect ResourceIdentifier excplicitly

Moreover, I can imagine such extension for any primitive type, not only string.

I'd be happy to contribute if you think it makes sense.

mnazbro commented 5 years ago

@iamdanfox Has this proposal been considered?

iamdanfox commented 5 years ago

:+1: think it looks sensible. Sadly we can't really do opt-in feature flags in this repo because this would allow API producers to ship compilation breaks in a minor release of their API.

The internal publish plugin uses the '301 versioning scheme' to mitigate this, so I think we'd just need to get this right first time and release a 4xx.

mcintyret commented 4 years ago

Agree that this would be very cool, although slight preference to not provide the of() method - since this resolves to a noop at runtime it feels unnecessary. Users could just do the cast themselves.

Que3216 commented 3 years ago

+1 on this issue. We've seen serious bugs that this would have prevented.

westinrm commented 2 years ago

189 addressed half of this since it generates flavored types for aliases, but it doesn't cover external types. Would extending #189 to support external types be reasonable?