facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.86k forks source link

"return this" from inherited method not working as expected #5925

Open kevanstannard opened 6 years ago

kevanstannard commented 6 years ago

I'm writing classes where the methods return "this" to allow for method chaining.

When the method is inherited then "return this" doesn't seem to be working as expected:

class A {
  one() { return this; } // "this" should return the instance of "B" when inherited
}

class B extends A {
  two() {}
}

const b = new B();

b.one(); // ok
b.two(); // ok
b.one().two(); // Cannot call `b.one().two` because property `two` is missing in `A`

On flow/try:

https://flow.org/try/#0MYGwhgzhAECC0G8BQ1oHsB2BTAFASkWgCcsAXAVyI2lIAsBLCAbmgF9oB6D6AIjsZ7QItNORAATYmUrU6WaPQwRSYDMHloAZrwBCggO60s1RUaL1SWcUlZIkoSDB3QsAD0sZxMeMlSl9aPiItrb2mMrQAEbQALzQ2PrQOvhMdpEAdJi4eCxc6ADWSBn+gTmc3GiFGVn46SUp5dAAwqoYaKTQwGAgINAABtXYtSV9UVhd5BDyAA5EaNNYRKQAnv0jCjAAtowQigDmCtR9sH12QA

Any suggestions appreciated, thanks.

aksuska commented 6 years ago

+1 this is really a false positive. The only workaround I know is the unfortunate use of // $FlowFixMe

kevanstannard commented 6 years ago

Many thanks @aksuska, will use that for now. I'll leave this open since it appears to be genuine issue.

noppa commented 6 years ago

I don't know why the type isn't inferred correctly there, but explicitly annotating the return type to be "this" works without having to suppress type safety.

Fixed example.

aksuska commented 6 years ago

That is working for me as well, though it doesn't quite seem right. What is the context for "this"? Will it vary whether the method declaration is static or not? On a different note, @noppa do you happen to know how to alias "this"?

aksuska commented 6 years ago

But this approach doesn't work for variable types

I get this is weird but I use polymorphic factory methods that may return the object called on (this) or another object of the same type.

noppa commented 6 years ago

@aksuska I don't quite follow, why do you need to alias it? I don't see it documented anywhere so I'm not sure, but it seems that the this type can only be used as the return type for a method.

Could you show an example where the type is difficult to declare correctly and that is a bit closer to your actual usecase?

When the factory is a static method of class A, this inside that static method will refer to the class that is used to invoke it, i.e. class A itself or some other class that extends it. If you want to return the class constructor itself, you could implement it like this:


class A {
  static factory(): Class<this> { return this; }
}
class B extends A {}

// Tests
const a: Class<A> = A.factory();
const b: Class<B> = B.factory();

flow.org/try.

Here, the return values are the classes itself, not instances of the classes, and type annotation Class<this> denotes that. However, if you are talking about a factory function in a more traditional sense and want to create instances from the class using a static method in the class, you would instead write something like


class A {
  static factory(): this { return new this(); }
}
class B extends A {}

// Tests
const a: A = A.factory();
const b: B = B.factory();

flow.org/try.

aksuska commented 6 years ago

The use case for alias is that I prefer to centralize often-used type declarations that serve a specific purpose. This way if the purpose changes, I don't have to search/replace through all my source files to update it. I can simply make the single change and it is reflected across my entire project codebase. It seems to me that the use of "this" might be a kind of accidental side effect to how Flow handles type declarations, which makes sense that it doesn't work where other type declarations easily work. The best solution. IMHO, is simply for Flow to properly type a prototype chain, which it seems to have difficulty with.