Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

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

Merged
merged 19 commits into from
Oct 20, 2022
Merged

Conversation

caass
Copy link
Contributor

@caass caass commented Oct 19, 2022

Closes #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, 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 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, 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" 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, does not allow const enum values, currently.

Here are two approaches to avoiding these pitfalls:

A. Do not use const enums at all. You can easily ban const enums 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. This is the approach taken internally by the TypeScript project itself. 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.

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
Copy link

changeset-bot bot commented Oct 19, 2022

🦋 Changeset detected

Latest commit: caf0a04

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

@caass caass marked this pull request as ready for review October 20, 2022 01:01
overrides/cf.d.ts Outdated Show resolved Hide resolved
@petebacondarwin
Copy link
Contributor

petebacondarwin commented Oct 20, 2022

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;

overrides/cf.d.ts Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
overrides/cf.d.ts Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix request.cf types
4 participants