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

disregard: typescript conversion #60

Closed
wants to merge 17 commits into from
Closed

disregard: typescript conversion #60

wants to merge 17 commits into from

Conversation

seankwarren
Copy link
Contributor

No description provided.

T extends Constructor<ApplicationBase> = Constructor<ApplicationBase>
>(superclass: T) {
return class AdeApplication extends superclass {
static Executable = Executable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error above and below happens because of static Flavor = Flavor and static Executable = Executable.
Is there a way to redesign this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only workaround I was able to find that works, is by restricting these Entities to classes rather than mixins. This means they cannot be mixed into another class while maintaining type safety, but can be much more easily attached to other classes similar to how Executable class is attached to Application class as a static attribute. It results in much more reasonable *.d.ts file sizes, and doesn't require maintaining class and interface separately. They should be overridable in web-app as long as each type is castable to the inferred type in the base Executable:
example: flavors getter expects to return an array. This means the base Executable definition should ensure that the type of each method is generic enough for our needs to support overriding.

export class WebappExecutable extends Executable {
    get flavors() {
        return [];
    }
}
export class WebappExecutableApplication extends Application {
    static Executable = WebappExecutable;
}

TLDR;

  • Use classes instead of mixins for Application, Executable, Flavor, Template, will need to reconsider ContextProviders since they will prefer mixin functionality.
  • Maintain base class interface as part of base class definition, overriding inferred types where necessary to be generic enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The application.d.ts file still seems very large. I will probably create a task for myself to investigate possible solutions for future work. Otherwise code looks good

getExecutableByName(name = null) {
return new this.constructor.Executable(
getExecutableByName(name?: string) {
return new Application.Executable(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that without this.constructor re-defining Executable in webapp startup will still work correctly??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work as expected

@seankwarren seankwarren reopened this Feb 28, 2024
@seankwarren seankwarren changed the title Feat/sof 7249 disregard: typescript conversion Feb 29, 2024
@seankwarren seankwarren deleted the feat/SOF-7249 branch February 29, 2024 19:13
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

Successfully merging this pull request may close these issues.

3 participants