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

Upgrade to TS 5.3 #7227

Open
na9da opened this issue Jun 28, 2024 · 3 comments
Open

Upgrade to TS 5.3 #7227

na9da opened this issue Jun 28, 2024 · 3 comments
Assignees

Comments

@na9da
Copy link
Collaborator

na9da commented Jun 28, 2024

TS 5.3 yet again breaks terriajs trait overrides in classes. Previously when upgrading to 4.x from 3.x we implemented a slightly hacky fix described under solution#2 here. However changes in 5.3 yet again breaks these overrides.

This issue is to discuss a more lasting solution for this. One approach is to use solution#1 from the other discussion.

Other ideas?

@pjonsson
Copy link
Contributor

pjonsson commented Jul 4, 2024

I don't know anything about TS, but in other typed languages (Haskell, C++/Java), the "type 1" errors would most often indicate something strange with the class hierarchy. Regarding the "type 1" errors caused by duplicate mixins, were the duplicate mixins removed from the hierarchy with #5219, or did "solution#2" let you keep the code as before?

The word trait is not mentioned in the release notes for TS 5.3 (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html), what change causes the problem with traits for TerriaJS?

I imagine that fewer type issues in the code in general makes it easier to upgrade; #7221 and #7218 both contain some fixes involving types.

@na9da
Copy link
Collaborator Author

na9da commented Jul 5, 2024

Just for context, traits are serializable properties of a model (eg: the ones you configure via catalog definitions). They are defined separately in standalone files and mixed in when defining the model. When mixing in, the traits are written to the class as JS get accessors and then we use TS interface to add those trait types to the class.

The problem happens when we later try to override some of these getter traits in the class definition. In JS & TS it is invalid to override a getter property with a field (or vice-versa) but we can still override a getter with another getter or a field with another field. The issue is that, because interfaces don't specify whether a field is a getter or normal property, TS will block our pattern of overriding a getter with a getter - because it assumes we are overriding a property with a getter.

This change was added in TS 4.x but we had a temporary fix that doesn't seem to work anymore. In this issue we are looking to find a more lasting solution to the problem.

@pjonsson
Copy link
Contributor

pjonsson commented Jul 5, 2024

Thank you for explaining. I'm not sure if it applies to your situation, and if it is a coincidence that you mention getters, but a quick search makes me believe an interface with a readonly property can have a getter override in a subclass: https://stackoverflow.com/a/41120071

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

3 participants