rancher / dashboard

The Rancher UI
https://rancher.com
Apache License 2.0
454 stars 257 forks source link

[TS] Convert base Resource model classes to typescript #5254

Open richard-cox opened 2 years ago

richard-cox commented 2 years ago

Types

Need to consider how we type...

richard-cox commented 2 years ago

@catherineluse There is a LOT of work here, so feel free to split it out into separate issues. There may also be design questions that need answering, like how to type Kubernetes resources, so feel free to reach out for a consensus on way forward.

Please note this should be done after 2.6.4 code complete

catherineluse commented 1 year ago

@cnotv @richard-cox I'm wondering if we should close this issue because I'm questioning if it's even possible to do this. When the UI instantiates a Resource class, it loops over a lot of keys from the Javascript object - the data about a specific resource - and applies them all to the Resource class. So the Resource could have any number of fields on it, in theory, and we would not be able to type them.

See this in the Resource constructor - this is what I think cannot be typed:

export default class Resource {
  constructor(data, ctx, rehydrateNamespace = null, setClone = false) {
    for ( const k in data ) {
      this[k] = data[k];
    }

I think that when we originally opened this issue, we were assuming that the Resource was generic, while the properties more specific to individual resources would be added in the classes that extend Resource. But that is not the case - a lot of very specific data is being added to Resource.

I don't see a way around that, but I'm open to suggestions. Maybe this idea https://github.com/rancher/dashboard/issues/6724 should be used to do a POC instead?

cnotv commented 1 year ago

I think #6724 is definitely more feasible and reliable, but this class will have to be extended in somehow or it will not make so much sense, no?

richard-cox commented 1 year ago

This is still 100% achievable and is at the core of converting all models to typescript.

cnotv commented 1 year ago

Hopefully for the interface is just about use a script to generate them from existing official classes, although it did not get updated since 6 months and we may reconsider the source.

To convert the resource is going to be more hand mad, so definitely more time, which is why I said "feasible".

catherineluse commented 1 year ago

For that part of the constructor, should Typescript be turned off for that part? Or should it be moved somewhere else?

cnotv commented 1 year ago

Should not be enough for data to be Partial<Resource> to address the issue?

cnotv commented 1 year ago

Example of attempted conversion to TS for a non-base model (Namespace) https://github.com/rancher/dashboard/issues/7981

cnotv commented 1 year ago

As we mentioned once again the need to type the fetched data, we should probably tackle this down: https://github.com/rancher/dashboard/issues/6724

cnotv commented 1 year ago

Also I add this as a comment, since a first attempt to migrate the resource has been done https://github.com/rancher/dashboard/pull/6106