-
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
disregard: typescript conversion #60
Conversation
T extends Constructor<ApplicationBase> = Constructor<ApplicationBase> | ||
>(superclass: T) { | ||
return class AdeApplication extends superclass { | ||
static Executable = Executable; |
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.
The error above and below happens because of static Flavor = Flavor
and static Executable = Executable
.
Is there a way to redesign this?
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.
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.
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.
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
src/application.ts
Outdated
getExecutableByName(name = null) { | ||
return new this.constructor.Executable( | ||
getExecutableByName(name?: string) { | ||
return new Application.Executable( |
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.
Did you check that without this.constructor
re-defining Executable in webapp startup will still work correctly??
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.
Seems to work as expected
No description provided.