Feat: foundation for TypeScript migration#7308
Conversation
- Previous upgrade from v8 -> v10 caused linter to break because of unsupported configuration files
| * var map = new Map([ | ||
| * [ 1, 'one' ], | ||
| * [ 2, 'two' ], | ||
| * [ 3, 'three' ] | ||
| * ]); |
There was a problem hiding this comment.
JSDoc version allowed numbers for keys, but it is confusing while using an object for storage internally. In JavaScript this will be true obj[1] === obj["1"], thus causing potential runtime bugs. Therefore I set the signature to Map<K extends string = string, V = unknown>.
| * | ||
| * @returns The callback result. | ||
| */ | ||
| type EachMapCallback<K extends string, V> = (key: K, entry: V) => boolean | void; |
There was a problem hiding this comment.
JSDoc declaration said this callback returns null when in reality it returns entries[key], not null. The type was misleading.
| { | ||
| "compilerOptions": { | ||
| "strict": true, | ||
| "target": "ES2018", |
There was a problem hiding this comment.
What would be a comfortable target without dropping too much compatibility? I understand there's probably varying views, but my point of view is that as a framework Phaser could ship builds with quite modern versions and people who want to support really old stuff can use existing tools in the ecosystem to convert to their preferred ES version. --- Also good to note we should definitely set the target to at least ES2015 to not have the distributables include bunch of utility code for e.g. classes
| export function composeMixins<const TMixins extends readonly Mixin<object>[]> ( | ||
| ...mixins: TMixins | ||
| ): <TBase extends Constructor>( | ||
| Base: TBase | ||
| ) => TBase & Constructor<InstanceType<TBase> & AddedByAll<TMixins>> | ||
| { | ||
| // The reduce chain is type-safe at call sites via the overload signature; | ||
| // the implementation needs a single cast to bridge the generic gap. | ||
| return ((Base: Constructor) => | ||
| mixins.reduce((Current, mixin) => mixin(Current), Base) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ) as any; | ||
| } |
There was a problem hiding this comment.
There's multiple ways to implement mixins, but this composable implementation keeps the footprint same size as before.
For comparison the worst case scenario with TS handbook mixins in current codebase would be:
// Note: creates many anonymous classes
const SpriteBase =
Alpha(
BlendMode(
Depth(
Flip(
GetBounds(
Lighting(
Mask(
Origin(
RenderNodes(
ScrollFactor(
Size(
TextureCrop(
Tint(
Transform(
Visible(
GameObject
)))))))))))))));
export class Sprite extends SpriteBase { ... }
But with the composable approach it would be:
// Note: creates only single class
const SpriteBase = composeMixins(
Alpha,
BlendMode,
Depth,
Flip,
GetBounds,
Lighting,
Mask,
Origin,
RenderNodes,
ScrollFactor,
Size,
TextureCrop,
Tint,
Transform,
Visible)(GameObject);
export class Sprite extends SpriteBase { ... }
| * The built-in `Object.keys()` always returns `string[]`, which loses | ||
| * generic key information. This helper returns `K[]` instead, making it | ||
| * safe to use the result to index back into the same record without casts. | ||
| * | ||
| * |
There was a problem hiding this comment.
I will mention that such a type-preserving Object.keys method should probably have a disclaimer about it being technically unsound due to structural typing allowing excess properties.
It's fine if the object in question is effectively nominal (i.e. we know it won't contain any extra keys), but should be avoided for anything where structural typing can get involved.
(NB: If this ends up being used a lot, it may make sense to declaration merge Object.keys and company to add this or something similar as an overload.)
| * `new (scene: Scene, type: string) => T` to the generic constraint. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| type Constructor<T = object> = new (...args: any[]) => T; |
There was a problem hiding this comment.
Suggested improvement, copied straight from type-fest:
type Constructor<T, Arguments extends unknown[] = any[]> = new (...arguments_: Arguments) => T(May want to look into adding it or a similar library as a dev dep for types - they've got some very useful utilities.)
| const record = value as Record<string, unknown>; | ||
|
|
||
| return typeof record.get === 'function' || typeof record.set === 'function'; |
There was a problem hiding this comment.
| const record = value as Record<string, unknown>; | |
| return typeof record.get === 'function' || typeof record.set === 'function'; | |
| return typeof (value as Record<string, unknown>).get === 'function' || typeof (value as Record<string, unknown>).set === 'function'; |
IMO, we should generally avoid creating temp variables for the compiler's sake where possible (type assertions are made for these kinds of things)
|
Just to mention that we are reading this, and are interested in doing it, but we have no capacity for such an undertaking for quite a while. At least until the end of August. Even if the community (thank you!) and let's face it, Claude, does the majority of the work, this change would have multiple tooling ramifications for us, well beyond simply updating the library and pushing to npm. Even without changing the API surface, this change would be profound. Also, if we're going to go down this path, we will almost certainly do it as part of a v5.0 release, to give us the chance to break the API in significant ways (i.e., to avoid the use of mixins). |
Introduction
This PR starts an incremental JavaScript to TypeScript migration path for Phaser. I acknowledge this will be an epic journey if we go on this path.
The motivation is related to #7298: the current declaration pipeline still depends heavily on
jsdoc, which has known parser limitations and an uncertain maintenance future. Rather than proposing an all-at-once rewrite, this branch shows that Phaser can migrate individual modules to native TypeScript while keeping the existing source tree, build output, and generatedtypes/phaser.d.tsworkflow intact.The migrated modules are intentionally small but varied, covering math helpers, a class, mixins, a struct, geometry, and a game object helper. They are now discovered automatically from migrated TypeScript source via JSDoc namespace tags, and the type generation step overlays authoritative TypeScript declarations for those migrated symbols into the existing Phaser declaration file.
Pros
tscas the source of truth for migrated declarations, avoiding somejsdocparser limitationstypes/phaser.d.tsoutput shape for consumersCons / Trade-offs
Technicalities
How distribution artifacts are stitched together
The published
phaser.d.tsremains a single output file, but during the migration period it is assembled fromtsgenoutput and an overlay of declarations emitted bytscfor migrated modules.flowchart TD jsFiles["JSDoc (.js files)"] --> jsdoc["jsdoc parser"] --> dtsDom["dts-dom tree"] tsFiles["TypeScript (.ts files)"] --> discovery["auto-discovery (JSDoc tags)"] --> tsc["tsc --emitDeclarationOnly"] dtsDom --> overlay["MigratedOverlay<br/>(synthetic stubs + overlay)"] tsc --> overlay overlay --> publish["publish.ts<br/>(orchestrator)"] publish --> phaser["phaser.d.ts"]Phases of the migration
npm run tsto update and validate the type declarations during the migration period. Once migrated modules reach 50% of source modules, it will also start warning that it may be time to consider a TS-first declaration pipeline. No concrete plan yet, but at that point most ofphaser.d.tswill be generated bytscand we should patch JSDoc definitions for the remaining JavaScript modules on top of thattsgencan be removed as declarations will be emitted bytscAgent skill for migrating modules
I wrote and tested an agent skill to help with the migration. This should be a great aid for contributors. You can find it here: https://github.com/niklas-e/phaser-typescript
TODO