Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subclassing is useless for generated classes #1

Open
mishok13 opened this issue Oct 5, 2017 · 2 comments
Open

Subclassing is useless for generated classes #1

mishok13 opened this issue Oct 5, 2017 · 2 comments

Comments

@mishok13
Copy link

mishok13 commented Oct 5, 2017

If we are trying to subclass from generated class we need to quite literally overwrite all modifying objects. To illustrate, I'll take your example from readme (with some minor modifications, such as _data having protected modifier instead of private):

export class Circle {
  // snipped for brevity
  protected _data: any;
  constructor(data: any = {}) {
    this._data = data;
  }
  get uuid(): string {
    return this._data.uuid;
  }
  setUuid(uuid: string): Circle {
    const data = this._data.uuid = uuid;
    return new Circle(data);
  }
  // More snipping
}

If we are to subclass from this object, say like this

class OtherCircle extends Circle {
  get foo(): string {
    return this._data.foo;
  }
  setFoo(foo: string): OtherCircle {
    const data = this._data.foo = foo;
    return new OtherCircle(data);
  }
}

Then trying to use this in the following piece of code will lead to type error:

let other: OtherCircle = new OtherCircle();
other = other.setUuid('bar'); // triggers TSError with message "Type 'Circle' is not assignable to type 'OtherCircle'."

Very often however we'll use const modifiers, which will mask the issue and lead to very nasty bugs later on:

const other: OtherCircle = new OtherCircle();
console.log(other instanceof OtherCircle); // true
const otherWithFoo = other.setFoo('bar');
console.log(otherWithFoo instanceof OtherCircle); // true, seems correct
const otherWithBar = other.setUuid('bar');
console.log(otherWithBar instanceof OtherCircle); // false !!!

I hope this explains what the issue is.

@mishok13
Copy link
Author

mishok13 commented Oct 5, 2017

I only have two ideas regarding this:

  • instead of using new ClassName use this.constructor when returning from setters
  • look into using generics

@mishok13
Copy link
Author

mishok13 commented Oct 5, 2017

Looking a bit more into second option, this seems like a peculiar case of https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern . Not sure if it can be translated directly into JavaScript (can't seem to find a clean way to do this tbh), but perhaps we could utilise microsoft/TypeScript#5949 if we are to go in that direction.

On the other hand, we could also make a drastic step and simply switch all our generated objects to be http://facebook.github.io/immutable-js/docs/#/Record This would most definitely require tons more work though and is definitely a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant