rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

[rush] Convert most enums to constants.

Open iclanton opened this issue 3 years ago • 3 comments
trafficstars

@elliot-nelson

Summary

This PR converts most enums in the rush-lib project to object constants with a merged type. This improves typesafety. For example:

import { BumpType } from '@microsoft/rush-lib';

const x: BumpType = BumpType.minor; // Works the same
const y: BumpType = 1234; // Used to be allowed, now errors

Issues

@octogonz - API Extractor isn't happy with the merged declarations. It has the following errors:

  [api-extractor] src\api\EnvironmentConfiguration.ts:22:14 - (ae-different-release-tags) This symbol has another declaration with a different release tag
  [api-extractor] src\api\EnvironmentConfiguration.ts:22:14 - (ae-missing-release-tag) "EnvironmentVariableNames" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
  [api-extractor] src\api\EventHooks.ts:12:14 - (ae-different-release-tags) This symbol has another declaration with a different release tag
  [api-extractor] src\api\EventHooks.ts:12:14 - (ae-missing-release-tag) "Event" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
  [api-extractor] src\api\PackageJsonEditor.ts:13:14 - (ae-missing-release-tag) "DependencyType" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
  [api-extractor] src\api\VersionPolicy.ts:26:14 - (ae-missing-release-tag) "BumpType" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
  [api-extractor] src\api\VersionPolicy.ts:59:14 - (ae-missing-release-tag) "VersionPolicyDefinitionName" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
  [api-extractor] src\logic\operations\OperationStatus.ts:9:14 - (ae-different-release-tags) This symbol has another declaration with a different release tag
  [api-extractor] src\logic\operations\OperationStatus.ts:9:14 - (ae-missing-release-tag) "OperationStatus" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)

How it was tested

This PR still needs to be tested.

iclanton avatar Nov 06 '22 01:11 iclanton

Looks good so far!

Played with the approach a bit in TS playground and it is doing what I want, specifically: it behaves rationally when interacting with supersets and subsets, like if I pass a BumpType into a function expecting 'major' | 'minor' | 'patch', I get the VSCode error Type "prerelease" is not assignable to type "BumpType", which is exactly what I want.

An advantage to this construct over a traditional union string is that if you add /** */ comments above each const property they show up on hover, which is nice.

EDIT: It looks like there's two different approaches you tried it; the const Foo = { } as const; is the one I think works better (the other one requires duplication of all property names afaict).

elliot-nelson avatar Nov 06 '22 16:11 elliot-nelson

API Extractor isn't happy with the merged declarations. It has the following errors:

I'm not sure this new syntax is an improvement. We should discuss it.

octogonz avatar Nov 06 '22 20:11 octogonz

API Extractor isn't happy with the merged declarations. It has the following errors:

I'm not sure this new syntax is an improvement. We should discuss it.

This syntax allows callers to continue to work as-is with the same type-safety as before (actually better, for any previously numeric enum), without depending on the TypeScript-specific "enum" feature. It's what we use internally as a drop-in replacement for const enum.

If targeting webpack 5, the optimal syntax gets a bit more complicated, because it becomes best to define each enum in its own file as a set of individual export const statements, then do:

import * as RawFooEnum from './FooEnum';
const FooEnum: typeof RawFooEnum = RawFooEnum;
type FooEnum = typeof RawFooEnum[keyof typeof RawFooEnum];
export { FooEnum };

This allows webpack to tree-shake out unused enum values.

dmichon-msft avatar Nov 16 '22 22:11 dmichon-msft

Closing this for now: this can be revisited in the future.

iclanton avatar Mar 04 '24 06:03 iclanton