Remove all TypeScript enums and forbid them in ESLint

[aloisklink]

📑 Summary

TypeScript enums are an unusual TypeScript feature, because it's one of the only features that is "not a type-level extension of JavaScript".

This means TypeScript generates custom code for enums, which can cause a bunch of issues, especially as this custom code can be built differently depending on which bundler you use, and because of this, many people discourage the use of enums:

• The Dangers of TypeScript Enums
• Tidy TypeScript: Prefer union types over enums
• TypeScript: Handbook - Enums # Objects vs Enums

In fact, even the official TypeScript handbook says:

Enums are a feature added to JavaScript by TypeScript which allows for describing a value which could be one of a set of possible named constants. Unlike most TypeScript features, this is not a type-level addition to JavaScript but something added to the language and runtime. Because of this, it’s a feature which you should know exists, but maybe hold off on using unless you are sure.

Taken from https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#enums, used under the CC BY 4.0 License.

I've also added an ESLint rule to ban TypeScript enums, using the no-restricted-syntax ESLint rule.

Better alternatives for TypeScript enums

For public APIs (e.g. that non-Mermaid users will use), it's better to use string literals, e.g.:

export type MyValue = "a" | "b" | "c";

For internal use, const assertions where added in TypeScript 3.4 and can be used to create enum-like objects in plain JavaScript, that act like TypeScript enums, but has none of the downsides of TypeScript enums.

Using a JavaScript object and as const performs pretty much exactly the same as a TypeScript enum, without the downsides of a TypeScript enum:

const MyEnum = {
  ABC: "my-string-val",
  DEF: "my-other-enum-val",
} as const;

📏 Design Decisions

Important: This is a breaking change to the MermaidConfig Sankey diagram type. However, since the Sankey diagram PR was only merged yesterday (see #4502), as long as this PR is merged before the next Mermaid release, this change will be fine.

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added unit/e2e tests (if appropriate)
  • ESLint rule added.
• 📓 have added documentation (if appropriate)
  • Warning added to ESLint rule.
• 🔖 targeted develop branch

[codecov]

Codecov Report

Merging #4580 (79688a1) into develop (9c011ab) will increase coverage by 4.75%.
The diff coverage is 90.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4580      +/-   ##
===========================================
+ Coverage    75.10%   79.85%   +4.75%     
===========================================
  Files          135      143       +8     
  Lines        16498    16499       +1     
  Branches       510      519       +9     
===========================================
+ Hits         12390    13176     +786     
+ Misses        3997     3218     -779     
+ Partials       111      105       -6     

Flag	Coverage Δ
e2e	84.01% <66.66%> (+8.45%)	⬆️
unit	58.08% <100.00%> (-2.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ages/mermaid/src/diagrams/sankey/sankeyRenderer.ts	85.18% <50.00%> (ø)
packages/mermaid/src/config.ts	72.61% <100.00%> (+2.21%)	⬆️
packages/mermaid/src/defaultConfig.ts	93.82% <100.00%> (ø)

... and 41 files with indirect coverage changes

[nirname]

@aloisklink Thank you for being attentive to details. As a non TS programmer I cannot fully appreciate all the nuances, but I have experience with other languages that support this.

Enums originated quite a time ago in static typed languages, and in majority of cases they are implemented as numeric constants under the hood. Like in c++:

enum Weekday {
  Monday, // 0
  Tuesday // 1
}

And such a notation it is absolutely valid (not checking syntax, though):

Weekday(0);

So the statement

enums are not a type safe

related not only to typescript, but rather to the the nature of enums. There are many cases when these could be type-casted dynamically to Int and vise versa, like in comparison operations for instance.

Speaking about string enums, there was no such a thing in older c++ versions (I am not following latest standards), so there was some tricks to do the same for strings, including but not limited to:

• writing macros, which is in fact a sub language of c++ (thats overcomplicated sometimes), so it would be interpolated to the result code in-place, or
• using string constants

None of the latter are memory-economic, so there was even some more evil hacks to achieve desired behavior.

I've read the articles you provided and from my background the arguments "to avoid them unless you are sure" as well as "not being type safe" are not strong enough. They behave almost exactly the same way as in other languages, and I consider it as expected behavior. Since there is a enum in typescript why not use it?

Nevertheless, I am OK "string constants" as a preferred way in that case. Perhaps, enums can confuse other developers who never faced it. Or we may run into an issue with it later on.

The main thing that can cause misunderstanding is that a user's configuration contains a JSON with string always, not a enum.

From my perspective (I hope I don't hurt anyone's feelings) the whole typescript is somewhat an imperfect patch trying to fix JavaScript "weakly" typed design (not unlike PHP) and resulting scalability problem. So it does not really matter to me which type to use. What matters is that it is better to choose one option and to stick with it for the whole project, thus your change that forbids enums is absolutely valid.

TLDR, summarize:

• I dont have personal preferences on what to use (enums, vs constans), I am ok with both
• Graph configuration is JSON with string always, not a enum, that can cause misunderstanding
• If we dont use enums, then forbid it (done), or stick to them whenever it is possible

@sidharthv96 as a main reviewer of Sankey diagrams, what do you think?

[aloisklink]

From my perspective (I hope I don't hurt anyone's feelings) the whole typescript is somewhat an imperfect patch trying to fix JavaScript "weakly" typed design

I agree. That's actually the main issue with TypeScript enums, it's a TypeScript-only feature that doesn't exist in JavaScript. Pretty much all modern TypeScript features are only type-level features. E.g., you can delete all the type information, and you'll be left with perfectly valid JavaScript code.

TypeScript enums were added to the language a long time ago, and are different because they require custom JavaScript code to be generated (there's even been talk about potentially deprecating it, although it's a bit unlikely since it will break a lot of legacy code).

There's even a proposal for enums to officially be added to JavaScript (see https://github.com/rbuckton/proposal-enum), and if that gets added, it might break existing TypeScript enum code, since they use the same syntax. (TypeScript enums are also excluded from the ECMAScript proposal: Type Annotations for this same reason).

It's generally safe to use TypeScript enum is private files, but if you're making a public API, then using a TypeScript enum usually isn't a good idea.

From a C++ perspective, imagine TypeScript enums to be a feature that isn't part of the official C++ standard, but something that's only supported in a specific C++ compiler.

What matters is that it is better to choose one option and to stick with it for the whole project

👍 That's good to hear! My vote is to remove them and forbid them, since otherwise, once we have them, we wouldn't be able to remove them without breaking backwards compatibility, but I'd love to get the comments from anybody else that has TypeScript experience.

[sidharthv96]

I think the given reasons are valid, and mermaid being a library that will be consumed by other developers, sticking to the core is the best option for us.

The changes in this PR making use of constants are simple and enhances the readability.
Especially in places like

where the []: syntax might confuse people.