denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.83k stars 5.39k forks source link

Support global type augmentations using `declare global` in JSR #23427

Open dsherret opened 7 months ago

dsherret commented 7 months ago

Global type augmentation has been banned in JSR as a temporary measure until we can figure out how to make it work well.

Some considerations for this feature:

  1. Global types can appear in any file.
    • Currently we figure out the public API by walking from the imported exports. Once the public API is determined, no more modules are walked for performance reasons. That means we could potentially drop files that have global types.
  2. We need a way to start at a global type augmentation and the figure out all the types used in that global augmentation (similar to how we tracing types through the exports of a package)
  3. A package may import an npm package that contains global types.
  4. A package may import another jsr package export that constains global types. If the other package export uses global types we shouldn't drop it from the type graph.
  5. We'd like this to not have a huge performance hit.

This is somewhat of a question of how much should package authors specify upfront vs how much of a perf hit do we want to take. Using our module analysis we could easily track which files use global types in a jsr package's export and then always include those files as part of the type graph to be sent to tsc (starting at the global augmentations within those files and then working out). The issue though is detecting npm packages that do global augmentations. We currently don't even parse npm package type definitions in Rust (we send that all directly to tsc) and doing so would have a negative impact on performance (since we'd need to parse the files and do all the module resolution within npm packages to search for global types). Alternatively, we could just include all npm packages when type checking, which is not a great solution for performance either.

Forcing package authors to adopt a convention wouldn't be terrible to do and would allow us to ensure complicance to the convention at publish time.

Proposed solution

This solution proposes strict requirements that we can loosen them over time:

  1. Globals must be stored in a globals.ts or globals.mts (ex. globals.ts, mod.globals.mts, web.globals.ts)
    • If a file contains declare global in another file, publishing will error
  2. These files must be imported by each entrypoint that uses them.
    • Roundabout access is not allowed (ex. mod.ts -> logger.ts -> web.globals.ts) unless also imported directly.
    • Multiple entrypoints may share the same globals
    • It is possible for one entrypoint to have globals and another entrypoint to not have globals.
  3. Any npm or jsr package imported by these files will also cause their globals to be used.

A danger with this approach is we don't want someone to publish a package that depends on globals of another package and then not have something work. This can be prevented by type checking public API of each export separately on publish to ensure there are no issues and then for certain compiler errors link to documentation about globals.

Note that this proposal only applies to global augmentation like declare global { ... } and not global augmentation with ambient modules (ex. declare module "node:fs" { ... }) or allowing the use of triple slash directives.

Overall, this solution is not great, but it should have good performance and we can always loosen the restrictions over time.

lionel-rowe commented 2 months ago

Note that this proposal only applies to global augmentation like declare global { ... } and not global augmentation with ambient modules (ex. declare module "node:fs" { ... }) or allowing the use of triple slash directives.

Does that mean there'd be no way to get it to work with JS source files?

polyfill.mjs:

// @ts-check
/// <reference types="./polyfill.globals.ts" />
import { regExpEscape as escape } from './escape.mjs'
Object.defineProperty(RegExp, 'escape', { value: escape, writable: true, enumerable: false, configurable: true })

polyfill.globals.ts:

declare global {
    interface RegExpConstructor {
        escape(str: string): string
    }
}

Or what about this version, with @type {import...} where the imported file contains global augmentation?

polyfill.mjs:

// @ts-check
import { regExpEscape as escape } from './escape.mjs'
/** @type {import('./polyfill.globals.ts')} */
Object.defineProperty(RegExp, 'escape', { value: escape, writable: true, enumerable: false, configurable: true })