phaserjs / phaserjs/phaser

Phaser `EventEmitter` typings are unsatisfactory and unsafe

Open
#7,249 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

🐛 TypeScript Bug
Dominant language
JavaScript
Stars
40.3k
Forks
7.2k
PR merge metrics
No merged PRs in 30d

Description

Version

  • Phaser Version: 3.90.0
  • Operating system: Ubuntu 24.04 LTS (platform agnostic issue)

Description

At the current moment, the types of Phaser.Events.EventEmitter and company look like this:

Phaser.Events.EventEmitter#once

This (especially the use of Function and any-typed callback scope) presents several pitfalls when working with the project in TypeScript:

  • No autocomplete or type safety for what event names are accepted by which emitter
    • This is only exacerbated by the properties of Phaser.Events all being typed as string, despite having canonical string names in the example code!
  • No inherent type safety or argument inference for events on callback types, requiring developers to flip back and forth between the API pages for trivial tasks

Example Test Code

import Phaser from "phaser";

const game = new Phaser.Game({type: Phaser.HEADLESS});

game.events.on("b00t", (arg1) => initializeGame(arg1)) // Oops! meant to put "boot"; this never runs
game.events.on("ready", (foo, bar) => {}) // no inference of the callback type

Additional Information

While this would be a fairly large change, it would be fairly routine to convert this code into being type safe.

  1. Add any necessary @const annotations to the types of all Phaser.Events.XYZ variables to ensure TypeScript emits them as their actual values instead of string or symbol.
  2. Convert EventEmitter into a generic EventEmitter<Events extends PhaserEventMap = PhaserEventMap class via JSDoc's @template tag. This ensures backwards compatibility for existing code that extended the generic class before it got a type parameter. (Moreover, code that wants to expand the allowed types of a given event map can extend said event map with their own constants.)
  3. Create unique "event map" interfaces for each instance of EventEmitter that is relied upon by currently functioning code. It is very important that these are interfaces, not types for the interface merging reasons highlighted above.
  4. Provide said event maps at callsites to specify what events each emitter produces.

Example snippet of what the .d.ts file could look like:

import type Phaser from "phaser";

type PhaserEventMap = {
    [key: string | symbol]: Function;
}

declare class EventEmitter<T extends PhaserEventMap = PhaserEventMap> {
    emit<E extends keyof T>(evt: E, callback: T[E], ...scope: ThisParameterType<T[E]> extends unknown ? [] : [ThisParameterType<T[E]>]): void
}

interface MyEventMap {
    'complete': (s: Phaser.Scene) => void;
    'completeWithScope': (this: 3, s: Phaser.Scene) => void;
}

// NB: We will have to use a type helper to convert from interfaces to types, as TS considers interfaces "closed"
// and requires explicit index signature declarations on anythign that implements EventMap when we really just want to check assignability
type Simplify<T> = { [KeyType in keyof T]: T[KeyType]; }

declare const e: EventEmitter<Simplify<MyEventMap>>;
e.emit("complete", (s) => s.add.arc(1, 2)); // callback has specified type of (Phaser.Scene) => void

Notably, the types accepted are already inside the code - we'd just have to convert the @param descriptions into some sort of map.

[!IMPORTANT]
I understand this would be a large change for consumers of existing packages, but this would only actually break those users using TypeScript who were previously passing incorrect callbacks. As such, I see no harm in tightening the compile-time checks to match the runtime ones.
I am willing to work on this myself, provided I have the time and get a go-ahead from someone about the approach.

Blocked by #7248

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the Phaser.Events.EventEmitter typings and the event-emitter call sites described in the issue, then inspect blocked issue #7248. Map existing event names and callback parameters before deciding the event-map boundaries. Done means the relevant emitters expose checked event names and inferred callback arguments without losing existing compatibility.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
developer-experience
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.