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

Add typescript friendly mixins #85

Merged
merged 3 commits into from
May 30, 2024
Merged

Add typescript friendly mixins #85

merged 3 commits into from
May 30, 2024

Conversation

jrobinson01
Copy link
Contributor

adds typescript ports of the routing mixins:
routing.mixin.ts and animated-routing.mixin.ts
This PR also includes the built .js versions of those files. These might play nicer with projects that are not using typescript, but are using tsc and jsdoc. This PR should not affect projects using closure compiler.

// routeExit(currentNode: RouteTreeNode, nextNode: RouteTreeNode | undefined, routeId: string, context: Context): Promise<boolean | void>;
// }

function animatedRoutingMixin<T extends Constructor<LitElement>>(Superclass:T, className:string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be specific to LitElement?

Suggested change
function animatedRoutingMixin<T extends Constructor<LitElement>>(Superclass:T, className:string) {
function animatedRoutingMixin<T extends Constructor<HTMLElement>>(Superclass:T, className:string) {

Copy link
Contributor Author

@jrobinson01 jrobinson01 May 24, 2024

Choose a reason for hiding this comment

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

I would think so, but there are some downstream (platform-tools) components that extend this mixin that complain when using LitElement HTMLElement. I know it's not ideal to be restricted to LitElement, but I don't believe that we'll be using any other library for the foreseeable future.

Choose a reason for hiding this comment

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

i commented below but without the LitElement as the extended class, it causes typescript errors when using the lifecycle methods. @barronhagerman @jrobinson01

routeEnter(currentNode: RouteTreeNode, nextNodeIfExists: RouteTreeNode | undefined, routeId: string, context: Context): Promise<boolean | void>;
routeExit(currentNode: RouteTreeNode, nextNode: RouteTreeNode | undefined, routeId: string, context: Context): Promise<boolean | void>;
}
const RoutingMixin = <T extends Constructor<LitElement>>(superclass: T) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Should this be specific to LitElement?

Suggested change
const RoutingMixin = <T extends Constructor<LitElement>>(superclass: T) => {
const RoutingMixin = <T extends Constructor<HTMLElement>>(superclass: T) => {

Choose a reason for hiding this comment

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

This was something we discovered in origin. If we don't use LitElement as the super class we get linter errors when using the lifecycle methods like connectedCallback etc

https://lit.dev/docs/composition/mixins/#mixins-in-typescript

@jrobinson01 jrobinson01 marked this pull request as draft May 24, 2024 18:54
@jrobinson01
Copy link
Contributor Author

jrobinson01 commented May 24, 2024

please don't merge yet. I found a bug, and hopefully a simpler way to do this. fixed

@jrobinson01 jrobinson01 marked this pull request as ready for review May 24, 2024 19:16
@jrobinson01 jrobinson01 merged commit df75e7e into master May 30, 2024
5 checks passed
@jrobinson01 jrobinson01 deleted the ts-mixins branch May 30, 2024 14:48
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.

None yet

4 participants