cloudflare / workers-types

TypeScript type definitions for authoring Cloudflare Workers.
BSD 3-Clause "New" or "Revised" License
359 stars 89 forks source link

begin implementing real real good types for request.cf #301

Closed caass closed 1 year ago

caass commented 1 year ago

Closes https://github.com/cloudflare/workers-types/issues/296

Resolved, we're using unions instead of enums for now There's an open issue around the use of `const enum`s to describe certain hardcoded values whose meaning may not be obvious to the user. There's issues around [using `const enum`s in `.d.ts` files](https://www.typescriptlang.org/docs/handbook/enums.html#const-enum-pitfalls). Relevant section quoted here: > Inlining enum values is straightforward at first, but comes with subtle implications. These pitfalls pertain to _ambient_ const enums only (basically const enums in `.d.ts` files) and sharing them between projects, but if you are publishing or consuming `.d.ts` files, these pitfalls likely apply to you, because `tsc --declaration` transforms `.ts` files into `.d.ts` files. > > 1. For the reasons laid out in the [`isolatedModules` documentation](https://www.typescriptlang.org/tsconfig#references-to-const-enum-members), that mode is fundamentally incompatible with ambient const enums. This means if you publish ambient const enums, downstream consumers will not be able to use [`isolatedModules`](https://www.typescriptlang.org/tsconfig#isolatedModules) and those enum values at the same time. > 2. You can easily inline values from version A of a dependency at compile time, and import version B at runtime. Version A and B’s enums can have different values, if you are not very careful, resulting in [surprising bugs](https://github.com/microsoft/TypeScript/issues/5219#issue-110947903), like taking the wrong branches of if statements. These bugs are especially pernicious because it is common to run automated tests at roughly the same time as projects are built, with the same dependency versions, which misses these bugs completely. > 3. [`importsNotUsedAsValues: "preserve"`](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) will not elide imports for const enums used as values, but ambient const enums do not guarantee that runtime `.js` files exist. The unresolvable imports cause errors at runtime. The usual way to unambiguously elide imports, [type-only imports](https://www.typescriptlang.org/docs/handbook/modules.html#importing-types), [does not allow const enum values](https://github.com/microsoft/TypeScript/issues/40344), currently. > > Here are two approaches to avoiding these pitfalls: > > A. Do not use const enums at all. You can easily [ban const enums](https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#how-can-i-ban-specific-language-feature) with the help of a linter. Obviously this avoids any issues with const enums, but prevents your project from inlining its own enums. Unlike inlining enums from other projects, inlining a project’s own enums is not problematic and has performance implications. > > B. Do not publish ambient const enums, by deconstifying them with the help of [`preserveConstEnums`](https://www.typescriptlang.org/tsconfig#preserveConstEnums). This is the approach taken internally by the [TypeScript project itself](https://github.com/microsoft/TypeScript/pull/5422). [`preserveConstEnums`](https://www.typescriptlang.org/tsconfig#preserveConstEnums) emits the same JavaScript for const enums as plain enums. You can then safely strip the const modifier from .d.ts files [in a build step](https://github.com/microsoft/TypeScript/blob/1a981d1df1810c868a66b3828497f049a944951c/Gulpfile.js#L144). > > This way downstream consumers will not inline enums from your project, avoiding the pitfalls above, but a project can still inline its own enums, unlike banning const enums entirely. I'm not sure what approach we want to take. I think if we're moving towards using importable types rather than ambient declarations, this becomes a non-issue. If we keep these declarations ambient, we should do something about it.
changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: caf0a045f4d264343ae2fa327a9b8a1afd82ebca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------------------- | ----- | | @cloudflare/workers-types | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

petebacondarwin commented 1 year ago

Regarding const enums... do these have to be enums at all?

Can they just be constants grouped in a namespace (i.e. an object)?

I assume that since you were using const enum there is no expectation that these enums would have all the other fancy features of Enums in general?

For example instead of:

declare const enum ContinentCode {
  Africa = "AF",
  Antarctica = "AN",
  Asia = "AS",
  Europe = "EU",
  NorthAmerica = "NA",
  Oceania = "OC",
  SouthAmerica = "SA",
}

Perhaps:

declare type ContinentCode = "AF" | "AN" | "AS" | "EU" | "NA" | "OC" | "SA";

declare const ContinentCode = {
  Africa: "AF",
  Antarctica: "AN",
  Asia: "AS",
  Europe: "EU",
  NorthAmerica: "NA",
  Oceania: "OC",
  SouthAmerica: "SA",
} as const;