Skip to content

genType: treat option<t> as t | undefined and remove special treatment of record fields of option type. #6022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Mar 1, 2023

Can be used to bind to functions with optional parameters, using option.

Before: option<t> maps to this TS null | undefined | t
Now: option<t> maps to this TS undefined | t

Before: {x:option<string>} maps to this TS {x?: string}.
Now: {x:option<string>} maps to this TS {x: (undefined | string)} (same for {x:Js.undefined<string>})
Unchanged: {x?: string} maps to this TS {x?: string}.

This also produces strictly fewer conversion functions, as it does not need to transform null to undefined.

This produces strictly fewer conversion functions, as it does not need to transform `null` to `undefined`.
@cristianoc cristianoc force-pushed the gentype_option_as_undefined branch from f5a3583 to 83404c1 Compare March 1, 2023 10:48
@cristianoc cristianoc changed the base branch from gentype_core to master March 1, 2023 10:49
@cristianoc cristianoc changed the base branch from master to gentype_core March 1, 2023 10:49
This is a breaking changes, but aligns more closely with TS.

The two TS types:

```ts
type t1 = { x?: string };

type t2 = { x: (undefined | string) };
```

now correspond to the ReScript types:
```res
@genType
type t1 = {x?: string}

@genType
type t2 = {x: Js.undefined<string>}
```

The special treatment of fields of option type comes from a time where records with optional fields did not exist.
@cristianoc cristianoc changed the title genType: treat option<t> as t | undefined genType: treat option<t> as t | undefined and remove special treatment of record fields of option type. Mar 1, 2023
@cristianoc
Copy link
Collaborator Author

cristianoc commented Mar 1, 2023

Added examples. TS functions:

// CoreTS.ts
export declare function someFunWithNullThenOptionalArgs(
  nullable: null | string,
  app?: string
): string;

export declare function someFunWithNullUndefinedArg(
  nullUndefined: null | undefined | string,
  other: number
): string;

ReScript bindings:

@genType.import("./CoreTS")
external someFunWithNullThenOptionalArgs: (
  Null.t<string> /* Cannot be Nullable.t or option */,
  option<string> /* Cannot be Null.t or Nullable.t */,
) => string = "someFunWithNullThenOptionalArgs"

@genType.import("./CoreTS")
external someFunWithNullUndefinedArg: (
  Nullable.t<string> /* Can also be Null.t or option as they are subtypes */,
  int,
) => string = "someFunWithNullUndefinedArg"

Notice that out of the 3 choices: (null | string), (undefined | string), (null | undefined | string), there is no freedom at all for binding to someFunWithNullThenOptionalArgs: one must use the first and second one respectively.
What this means is that one needs to have a way to express each of the 3 choices.
With this PR, option can be used to bind to TS optional arguments, and Undefined.t becomes redundant.

@cristianoc
Copy link
Collaborator Author

Complete bindings for one firebase function (CC @jmagaram):

type firebaseOptions = {
  apiKey?: string,
  authDomain?: string,
  databaseURL?: string,
  projectId?: string,
  storageBucket?: string,
  messagingSenderId?: string,
  appId?: string,
  measurementId?: string,
}

type firebaseApp = {
  name: string,
  options: firebaseOptions,
  automaticDataCollectionEnabled: bool,
}
type analytics = {app: firebaseApp}
type analyticsCallOptions = {global: bool}

@genType.import("firebase/analytics")
external setUserId: (analytics, Null.t<string>, option<analyticsCallOptions>) => unit = "setUserId"

gives

/* TypeScript file generated from Core.res by genType. */
/* eslint-disable import/first */


import {setUserId as setUserIdNotChecked} from 'firebase/analytics';

// In case of type error, check the type of 'setUserId' in 'Core.res' and 'firebase/analytics'.
export const setUserIdTypeChecked: (_1:analytics, _2:(null | string), _3:(undefined | analyticsCallOptions)) => void = setUserIdNotChecked;

// Export 'setUserId' early to allow circular import from the '.bs.js' file.
export const setUserId: unknown = setUserIdTypeChecked as (_1:analytics, _2:(null | string), _3:(undefined | analyticsCallOptions)) => void;

// tslint:disable-next-line:interface-over-type-literal
export type firebaseOptions = {
  readonly apiKey?: string; 
  readonly authDomain?: string; 
  readonly databaseURL?: string; 
  readonly projectId?: string; 
  readonly storageBucket?: string; 
  readonly messagingSenderId?: string; 
  readonly appId?: string; 
  readonly measurementId?: string
};

// tslint:disable-next-line:interface-over-type-literal
export type firebaseApp = {
  readonly name: string; 
  readonly options: firebaseOptions; 
  readonly automaticDataCollectionEnabled: boolean
};

// tslint:disable-next-line:interface-over-type-literal
export type analytics = { readonly app: firebaseApp };

// tslint:disable-next-line:interface-over-type-literal
export type analyticsCallOptions = { readonly global: boolean };

@cristianoc cristianoc merged commit bd8ad8b into gentype_core Mar 2, 2023
@cristianoc cristianoc deleted the gentype_option_as_undefined branch March 2, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant