-
Notifications
You must be signed in to change notification settings - Fork 362
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
Upgrade terriajs to typescript 4.x #5219
Comments
I want logical nullish assignment! 💥 |
Thought that microsoft/TypeScript#41994 might solve this issue, but just got the same error using it (maybe someone else wants to test it) |
Thanks @zoran995 for following it up with the TS team. I was thinking if we can find a workaround for this particular error. When building terriajs with TS4.x we have around 256 errors out of which 223 are TS2611 and out of them 132 happens in I was looking at the particular case of
Even though In LoadableStratum you can see that the factory function doesn't do much other than defining getters/setters for the traits and returning an abstract class which we inherit from. Instead of using inheritance to achieve this we could use a decorator function to enhance our Loadable stratum definition. Here is an example implementation that I think might work although I haven't fully tested it.
So we might be able to solve the TS2611 errors for |
Ideas
... |
Why TS2611
TS is trying to stop shooting ourselves on the foot by refusing to compile the above JS code that can lead to silent bugs. What changed in TS4.8From version 4.8, TS suppresses this error when the overridden property is defined as The good news is, this fixes a ton of errors for us when overriding traits in However we have around 30 remaining errors when overriding traits in Remaining TS2611 errors:
The error on Why does it fix
|
I had another go at this. I tried two approaches: Explicitly type all Mixin return valuesI started off with this approach but soon found issues which made me think that it is a no-go option. As explained under Fix#1 in the above comment, we explicitly type all Mixin returns by defining an interface: terriajs/lib/ModelMixins/TableMixin.ts Lines 66 to 93 in 356b053
Issues:
I think explicitly typing the Mixin return values is dangerous because it can hide
I think by not fully understanding the repercussions, we will work ourselves into a corner if we go with this approach Ignore unsolvable TS-2611 errorsIn this second approach (available here) I did the following:
We could gradually move some of these overrides to a loadable stratum where it makes sense or provide some helper mechanism to define it from within the class. I feel this is the best approach we have now because we won't be committing to a change we don't understand completely. Further, if we want to get rid of Note that TS 4.8 breaks some of our dependencies so to get it to fully compile without errors we have to enable This will also break other maps which we will have to test one by one. |
Update Nov 22:
|
Connected to #5866 |
Can we avoid publishing a new mobx-react by overriding some types in a .d.ts file? (Like we do for some Cesium types) |
Notes from out meeting yesterday on changing property access to functions:
Experimentation:
Even using:
the problem still exists |
Override system using endomorphisms - "mapping functions"
Disadvantages of
|
Update: So far we have two solutions [1, 2] for upgrading to TS4.x which we describe below. Any solution for TS2611 should also be compatible with mobx6. Also note that we are trying to get rid of the use of On a closer analysis, there are two kinds of TS2611 errors:
The basic trigger for both errors is overriding accessors that has multiple base declarations. Type 1 errorsThese happen when some Mixin enters the mixin hierarchy more than once. For eg, These errors can be fixed by removing duplicate Mixins from the hierarchy. Type 2 errorsFundamentally, these are also triggered when overriding some trait which has more than one base declaration. Eg, Unlike type 1 error, there is no easy way to remove the multiple base declarations in this case. We have implemented 2 solutions to fix TS2611 errors when overriding traits. Solution 1Define a hook at each class level that returns the trait overrides. Eg from GeoJsonMixin: terriajs/lib/ModelMixins/GeojsonMixin.ts Lines 337 to 355 in 38b61aa
Any mixin or catalog item can define the hook Pros:
Cons:
Thanks @kring for working out this solution. Solution 2Typescript 4.9 introduced an escape hatch for TS2611 errors. When an accessor has multiple base declarations, then the error is exempted if any 1 of those base declaration comes from an interface definition. That means we can fix the errors by declaring an interface with the same name as the trait classes. Eg for UrlTraits: terriajs/lib/Traits/TraitsClasses/UrlTraits.ts Lines 28 to 34 in 1a22955
Note that for this solution to work, both the class and the interface must be defined in the same module. So if an end developer wants to override a new trait that is not part of the interface definition in
Pros:
Cons:
Note that both implementations now use mobx6 and can use the new |
Update 1 Feb 2023:
|
Update 15 Feb 23:
|
Update 26 Apr:
|
Done. Please refer to upgrade guide for info on upgrading. |
Currently when using typescript 4.x with terriajs, we get a bunch of error TS2611. This ticket is to find a solution for the error and upgrade TS to the latest 4.x version.
The text was updated successfully, but these errors were encountered: