iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
83 stars 23 forks source link

Alert: Please export type for AlertProps.type #65

Closed DanEastBentley closed 3 years ago

DanEastBentley commented 3 years ago

Instead of this inline type for AlertProps.type:

    /**
     * Type of the alert.
     * @default 'informational'
     */
    type?: 'positive' | 'warning' | 'negative' | 'informational';

Can the type be exported:

export type AlertType = 'positive' | 'warning' | 'negative' | 'informational';

export type AlertProps = {
  /**
   * Type of the alert.
   * @default 'informational'
   */
  type?: AlertType;

We have introduced a function to convert our "MessageSeverity" enum to the Alert type:

export function getAlertType(severity: MessageSeverity): string {. . .
const alertType = getAlertType(severity);

We would rather use an exported type rather than a string as the return type. The string causes us to have to use "as any" in our Alert component usage:

<Alert type={alertType as any} onClose={handleClose}>
mayank99 commented 3 years ago

Hi Dan,

I think the problem is that you are explicitly telling Typescript that your function returns a string.

export function getAlertType(severity: MessageSeverity): string {
                                                         ^^^^^^

You should be able to remove : string and rely on TS to infer the type correctly since this is a simple use case. If that doesn't help, you can also try const assertion (introduced in TS 3.4).

Does that work for you?

mayank99 commented 3 years ago

After some more playing around, it seems that the string type also comes from the enum, so that won't work if you directly return an enum key.

If that is the case, try this:

enum CustomAlertEnum { positive, warning, negative, informational }; // assuming this already exists

type CustomAlertType = keyof typeof CustomAlertEnum;
DanEastBentley commented 3 years ago

I was able to solve it by using this:

import { MessageSeverity } from "@bentley/ui-core";

/** @internal */
export type AlertType = "positive" | "warning" | "negative" | "informational" | undefined;

/** @internal */
export function getAlertType(severity: MessageSeverity): AlertType {
  let alertType: AlertType;

  switch (severity) {
    case MessageSeverity.Information:
      alertType = "informational";
      break;
    case MessageSeverity.Warning:
      alertType = "warning";
      break;
    case MessageSeverity.Error:
    case MessageSeverity.Fatal:
      alertType = "negative";
      break;
    case MessageSeverity.None:
    default:
      alertType = "positive";
      break;
  }

  return alertType;
}

With that change, the "as any" is not required. However, I must note that requiring a consumer to define the AlertType type in this manner is not ideal.

mayank99 commented 3 years ago

The problem there again is using let to create a variable, so Typescript infers it as a string at first assignment.

Directly returning those string literals works:

export function getAlertType(severity: MessageSeverity) {
  switch (severity) {
    case MessageSeverity.Information:
      return "informational";
    case MessageSeverity.Warning:
      return "warning";
    case MessageSeverity.Error:
    case MessageSeverity.Fatal:
      return "negative";
    case MessageSeverity.None:
      return "positive";
    default:
      return undefined;
  }
}

image

If you have a linter rule that requires return types on a function, you can use AlertProps['type'] with that same code:

export function getAlertType(severity: MessageSeverity): AlertProps['type'] {

We want to avoid exporting types where not needed. If we export this, it sets the precedent for exporting other similar union types in the future.