-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Polymorphic/generic this as return type for subclasses #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f1ames
changed the title
Polymorphic/generic this as return type for subclasses (WIP)
Polymorphic/generic this as return type for subclasses
Nov 5, 2021
ErykSol
approved these changes
Nov 8, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!!!
jan-warchol
approved these changes
Nov 9, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!! 👍 🥇
This commit is trying to solve the issue of "polymorphic generic this as a return type". For more in-depth description see: #28. This issue is related to a fact that in `DataStream<T>` class we have multiple methods (transfroms) which returns `DataStream<U>` (so the instance of the same class, but with different type parameter). This works fine for `DataStream` itself but causes problems for derived classes as a return type is not dynamic and inhertied methods called on an instance of derived class will always return the instance of a parent class. First change was to introduce `.create()` method which is used inside tramsforms directly instead of calling `new DataStream...`. This allows derived classes to override it and have control over how its instances are created. With `.create()` in use, new instance based on call context (current this) is created. This results with a correct return type during runtime. The second change was to override transforms signatures in the derived class so correct return type could be declared. Those overriden transforms call `super.transform(...)` internally so the logic stays exactly the same. This solves the issue of TS not correctly recognizing return type (as without it, method signatures declare base class as return type). Above works well for generic derived classes. However, for non-generic derived classes (like `StringStream` in our case), overridden transform signatures (and also `.create()` method) needs to be non-generic as well to work with this approach. And so the third change was to overload transforms in a base class to have both non-generic and generic versions.
f1ames
force-pushed
the
task/generic-this
branch
from
November 10, 2021 09:05
4032f55
to
e05c530
Compare
Moved |
Rebased onto latest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I know this PR title sounds a little cryptic because well... it's not easy to explain but I will try.
The issue
The main issue this PR is trying to solve is as follows. Image we have a generic base class which methods can return new instances of itself:
And extending class:
So now any call to
derivedClassInstance.methodX()
should returnDerivedClass
instance. Unfortunately, with the code above it will always returnBaseClass
instance:Approaches
I spend some time looking into what we can do and how to handle this. I will talk a bit about some ideas.
Let's solve this with
this
If you look back at the above code,
BaseClass.method1()
could be written as:This partially solves the issue as calling
dc.method1(2);
will now returnthis
(which isdc
in runtime). However:.method2()
above) it's not really working.dc.method1()
returnsBaseClass<T>
as method signature says.dc.method1().test()
(even though it works in the runtime).Dynamic approach using TS utility functions
The idea was to somehow dynamically generate return type with TS
ReturnType
utility, something like:It doesn't have to be
this.constructor
it could be any property or method of the class we can fully control. However, using this in such context is prohibited by TS -Return type of public method from exported class has or is using private name 'this'.
And AFAIU this basically make it impossible to use any construct withthis
(so any field, method, etc) to dynamically type methods return type. Andthis
is the only thing we could rely on to deduce type here.Static approach and some typing
Since TS is statically typed language I tired more static approach with a lot of typing (typing as on keyboard)... 😝 Anyway...
It requires methods overloading and duplicating them in subclasses. This is the approach which lead me the closest to a working solution.
The solution
From what I understand what we want to achieve is called "polymorphic generic this" or higher-kinded types (see 1, 2 and 3) 🤔 I'm not sure though 🤔
And as for the above I've found two (related) issues in TypeScript tracker which describes similar/same concepts - microsoft/TypeScript#6223 and microsoft/TypeScript#4967 (comment). AFAIU it's not really feasible to achieve this with clean and more dynamic approach.
There are in fact two issues to be solved -
this
and return type should be a correct class instance during runtime. And TypeScript compiler should also understand the code and correct return type (so it can compile the code without errors and generate correct TS typings). This are in fact two separate issues where one doesn't necessary solve the other.This PR contains
src/generic-this-1.ts
file which shows the approach I have used on abstract classes. And so:Returning correct class instance during runtime
Instead of creating new instance directly in "common" methods, let's extract
create
method like:then in derived class we can simply override it so it creates instance of this class:
and now
DerivedClass
instance will be correctly returned:Make TS understand what's going on
Still TS will complain that
dc.method1(456);
returnsBaseClass<T>
and so we need to overridemethod1
like:And now both during compile time and runtime we have correct return type. It also generates correct TS typings.
Wait... there is more
What if we have derived class with "fixed" type:
This requires a similar approach but since
create
andmethod1
will not be generic here, it needs correct overloading in BaseClass:and then in derived class we use only non-generic version:
Now
BaseClassFixedType
instance will be correctly returned:Please see
src/generic-this-1.ts
file which contains the full code with sample calls too.Wait... there is more
Kind of... 😅 The issue I found so far with the above approach is that fixed derived class can't use generic overloaded method version. So the below code will fail:
The above is reflected in changes done in
DataStream
andStringStream
classes. And well, it works 🎉This PR doesn't solve the issue with mutating stream in/out types (as described in #23 (comment)) but it is a necessary "intro" we need to have to solve it correctly. That's also the reason why
map
was left untouched for now.Unit tests introduced in this PR checks both runtime types (
instanceof
andconstructor.name
) as well as compile time types (by trying to call.split()
method on resulting stream which fails if it is not aStringStream
decalred return type).