PAIR-code / megaplot

Apache License 2.0
19 stars 5 forks source link

TextSelection<T>.on*() methods have return type Selection<T> instead of TextSelection<T>, inhibiting chaining #73

Closed RyanMullins closed 1 year ago

RyanMullins commented 1 year ago

I encountered an error TS2739: Type 'Selection<Label>' is missing the following properties from type 'TextSelection<Label>': text, align, verticalAlign when trying to initialize a TextSelection using Scene.createTextSelection() with the code below.

import {Scene, SpriteView, TextSelection} from 'megaplot';

interface Label { /* some props */ }

interface Selections {
  labels: TextSelection<Label>;
}

function dummyOnExit(sprite: SpriteView, datum: Label) {}

class SomeClass {
  private readonly scene: Scene;
  private readonly selections: Selections;

  constructor(container: HTMLElement) {
    this.scene = new Scene({container});
    this.selections = {
      // This assignment will throw a TS2739 compiler error
      labels: this.scene.createTextSelection<Label>()
          .align(() => 'center')         // Returns TextSelection<Label>
          .verticalAlign(() => 'bottom') // Returns TextSelection<Label>
          .onExit(dummyOnExit);          // Returns Selection<Label>
    }
  }
}

This is caused by the interface declarations for the Selection<T> and TextSelection<T> types. Unlike conventional JavaScript class ... extends, where you could annotate the returns with this and the types would automatically update, TypeScript interface ... extends copies the exact definitions from the parent class without updating any of the types for the child. Thus, since the interface TextSelection<T> extends Selection<T> only declares the text, align, and verticalAlign properties, all of the other properties (e.g., onExit) retain the typing from Selection<T>.

This will induce a errors in two scenarios:

  1. A TS2739 error when assigning the result of TextSelection<T>.on*() to a property with type TextSelection<T>.
  2. A TS2339 error when chaining .align() etc. methods with the result of TextSelection<T>.on*().

None of the demos capture this because 1) they don't use method chaining syntax, and 2) most of them are written in JavaScript not TypeScript.

jimbojw commented 1 year ago

Thanks for finding and fixing this. 🙏