Skip to content
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

Replace enums with string literals #324

Closed
15 tasks done
michael-yx-wu opened this issue Dec 6, 2016 · 15 comments
Closed
15 tasks done

Replace enums with string literals #324

michael-yx-wu opened this issue Dec 6, 2016 · 15 comments

Comments

@michael-yx-wu
Copy link
Contributor

michael-yx-wu commented Dec 6, 2016

Let's replace enums with string literals. Instead of something like

export enums RegionCardinality {
  CELLS,
  FULL_COLUMNS,
}

we can have something like

export type RegionCardinality = "cells"
  | "full-columns"

export const RegionCardinality = {
  CELLS: "cells" as RegionCardinality,
  FULL_COLUMNS: "full-columns" as RegionCardinality,
}

@adidahiya pointed out that this may break existing apps so I'm adding this to the 2.x milestone for now. See #307 for existing PR. We can reopen it once we're ready to make this change.

List of enums:

  • Intent
  • Position
  • Table RegionCardinality
  • Elevation (cards)
  • AnimationState (Collapse)
  • CollapseFrom (CollapsibleList)
  • HotkeyScope
  • PopoverInteractionKind
  • DateRangeBoundary / RangeIndex
  • Months
  • TimePickerPrecision
  • Direction (Table)
  • RenderMode (Table)
  • QuadrantType (Table)
  • Orientation (Table)
@TomMarius
Copy link

What is the reason for this change?

@jkillian
Copy link
Contributor

jkillian commented Jan 2, 2017

It makes things faster to use/hack on. It also still is fully type-checked if you're using TypeScript.

For example, instead of:

import { Button, Intent } from "@blueprintjs/core";
// later
<Button intent={Intent.DANGER}>Delete</Button>

you could simplify to:

import { Button } from "@blueprintjs/core";
// later
<Button intent="danger">Delete</Button>

Of course, doing things the first way will still work as well, so no existing code should break. (Okay, it's possible some code could break depending on how enums were used in type positions or if people were doing crazy things with the numerical values of the enum...)

@TomMarius
Copy link

These examples unfortunately aren't equal in terms of usability and speed. String operations are generally slower than enum (number) operations. You can't easily refactor a string literal, but you can for example hit F2 on an enum member and rename all occurrences, and more little things like this. Why not use these things for its intended use cases?

@giladgray
Copy link
Contributor

the idea behind moving to typed string literals is that they can be used either as Intent.DANGER or as "danger" if you want to skip the import. the Intent export will still be there, but it'll be a map of string-to-string instead of string-to-number.

the performance effect of string vs number comparison is negligible; it's not useful to think like that.

@hellochar
Copy link
Contributor

Here's a cool trick in ts2.1 (from https://basarat.gitbooks.io/typescript/docs/types/literal-types.html):

/** Utility function to create a K:V from a list of strings */
function strEnum<T extends string>(o: Array<T>): {[K in T]: K} {
  return o.reduce((res, key) => {
    res[key] = key;
    return res;
  }, Object.create(null));
}

/**
  * Sample create a string enum
  */

/** Create a K:V */
const Direction = strEnum([
  'North',
  'South',
  'East',
  'West'
])
/** Create a Type */
type Direction = keyof typeof Direction;

@giladgray
Copy link
Contributor

@seansfkelley
Copy link

seansfkelley commented Jun 1, 2017

This also makes the API nicer for vanilla Javascript devs, where enumeration types like this aren't as idiomatic as they are in Typescript. https://github.com/dphilipson/typescript-string-enums is handy (I use it), and Typescript is adding string enumerations as a first-class thing in 2.4 (microsoft/TypeScript#15486).

Also, it makes it safer to use "|| default" idiom because you don't have to be careful of the 0-valued enumeration as (e.g.) Position.TOP_LEFT is: this.props.position || Position.BOTTOM.

@TomMarius
Copy link

TypeScript 2.4's string enums are now definitely the way to go.

@giladgray giladgray modified the milestones: 2.x, 2.0.0 Jul 13, 2017
@UselessPickles
Copy link
Contributor

UselessPickles commented Dec 6, 2017

+1 for string enums: https://www.typescriptlang.org/docs/handbook/enums.html

Make it a const enum to make it even lighter weight (no code emitted by typescript compiler; simply replaces all usages of the enums values with string literals during compile)

export const enum RegionCardinality {
  CELLS = "cells",
  FULL_COLUMNS = "full-columns"
}

(edit: On second thought, you may not want a const enum. A non-const enum will emit code that allows javascript projects to reference your enums by name. Const enums are more relevant for pure typescript projects)

They are essentially interchangeable with string literal types, but have the benefit of being organized, providing a place to document each value, and providing more explicit code completion in IDEs.

@giladgray
Copy link
Contributor

@UselessPickles ah didn't know about that const trick, but I think you're right that we do want to emit an enum type that can be used in code.

@giladgray giladgray self-assigned this Dec 11, 2017
@UselessPickles
Copy link
Contributor

@giladgray Yeah, the const trick makes much more sense in a purely TypeScript project (not a library to be used in other projects), or only for enums within a library project that are private/internal to the library.

const enums will be more useful once this bug is fixed: microsoft/TypeScript#17372

@UselessPickles
Copy link
Contributor

UselessPickles commented Dec 12, 2017

Another thing worth being aware of is that string literals are NOT assignable to enum types, even if the string literal exactly matches one of the enum's values. By defining something as an enum, you are forcing all TypeScript users to use the enum instead of a string literal (not a bad thing, but worth being aware of).

See this issue (marked "working as intended"): microsoft/TypeScript#15930

However, enum values ARE assignable to string literal types of matching values.

Code examples:

/**
 * General concept of region cardinality described here.
 * Individual values cannot be documented/explained clearly here.
 */
export type RegionCardinality = "cells" | "full-columns";

/**
 * General concept of region cardinality described here.
 */
export enum RegionCardinalityEnum {
  /**
   * Documentation explaining CELLS cardinality
   */
  CELLS = "cells",
  /**
   * Documentation explaining FULL_COLUMNS cardinality
   */
  FULL_COLUMNS = "full-columns"
}

function acceptsStringLiteral(cardinality: RegionCardinality): void {}
function acceptsEnum(cardinality: RegionCardinalityEnum): void {}

acceptsStringLiteral("cells"); // valid
acceptsStringLiteral(RegionCardinalityEnum.CELLS); // valid

acceptsEnum("cells"); // COMPILER ERROR!!!
acceptsEnum(RegionCardinalityEnum.CELLS); // valid

I personally don't see much value in string literal types now that string enums are available, though. My vote is for string enums only.

@TomMarius
Copy link

@UselessPickles That's good, actually. If the enum changes, the strings wouldn't update.

@UselessPickles
Copy link
Contributor

Tangentially related: I've been working on a little project to implement a generic visitor pattern that works on any string literal enum type, and any string literal union type. It provides a compile-time guarantee that you handle every possible value, including null/undefined when relevant.

I just finished cleaning it up and documenting it last night. Check it out: https://github.com/UselessPickles/ts-string-visitor

Available as an NPM package: https://www.npmjs.com/package/ts-string-visitor

@adidahiya
Copy link
Contributor

fixed by #1921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants