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

Allow refining the type of Controller elements #529

Merged
merged 1 commit into from
May 1, 2022

Conversation

rik
Copy link
Contributor

@rik rik commented Mar 19, 2022

Use generics to avoid having to repeat type assertions.

This can be used as is:

class MyFormController extends Controller<HTMLFormElement> {
    submit() {
        new FormData(this.element)
    }
}

The default value for that generics means you can still write class MyController extends Controller {}.


This was inspired by this conversation.

Use generics to avoid having to repeat type assertions.

This can be used as is:
```ts
class MyFormController extends Controller<HTMLFormElement> {
    submit() {
        new FormData(this.element)
    }
}
```

The default value for that generics means you can still write `class MyController extends Controller {}`.
@alexander-schranz
Copy link
Contributor

Please get this into it 🙏 . /cc @dhh :)

@marcoroth
Copy link
Member

marcoroth commented Apr 9, 2022

Hey @rik, thank you for opening this pull request!

I agree that it makes sense that you can refine the type of the controller element with generics in TypeScript. The use of the default value also seems super straight-forward, I like that!

Apart from your code changes, I think it would make sense that we add a chapter to the docs which outline some details on how you can use/integrate Stimulus with TypeScript. I've seen a few snippets in the past which might be worth to collect in a dedicated chapter in the docs, including this change here.

Would you mind starting such a chapter in this PR for this feature, so that we have a foundation to build on for other TypeScript related docs snippets in the future?

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 9, 2022

Maybe I can help here out:

# Using Typescript

Stimulus itself is written [TypeScript](https://www.typescriptlang.org/)
and provides types directly over its package.
The following documentation should make it clear how to define types for
stimulus properties.

## Define Controller Element Type

By default the `element` of the controller is from type `Element`.  
If the element expected type is as example `HTMLFormElement` it can
be defined this way:

```ts
import { Controller } from '@hotwired/stimulus';

export default class MyController extends Controller<HTMLFormElement> {
    submit() {
        new FormData(this.element)
    }
}
```

## Define Controller Value Type

To define the type of an configured values is possible
over the declare feature of TypeScript:

```ts
import { Controller } from '@hotwired/stimulus';

export default class MyController extends Controller {
    static values = {
        code: String,
    };

    declare readonly codeValue: string;
}
```

> The `declare` avoids override of the exist stimulus property and just define the type for TypeScript.

## Define Controller Target Type

To define the type of an configured targets is possible
over the declare feature of TypeScript:

```ts
import { Controller } from '@hotwired/stimulus';

export default class MyController extends Controller {
    static targets = [
        'input'
    ];

    declare readonly inputTarget: HTMLInputElement;
}
```

> The `declare` avoids override of the exist stimulus property and just define the type for TypeScript.

## Custom properties amd methods

Other custom properties is possible to define the TypeScript way
on the controller class:

```ts
import { Controller } from '@hotwired/stimulus';

export default class MyController extends Controller {
    container: HTMLElement;
}
```

Read more about it in the [TypeScript Documentation](https://www.typescriptlang.org/docs/handbook/intro.html).

@dhh dhh requested a review from marcoroth April 30, 2022 11:51
Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Thanks @rik, this is good as is.

Also thanks to you @alexander-schranz. Would you mind posting this as a separate PR so you can get the proper credit for it? Thank you!

@alexander-schranz
Copy link
Contributor

@marcoroth #540 here we go :)

@rik rik deleted the configurable-element-type branch May 2, 2022 12:20
@ReazerDev
Copy link

Hi, any ETA for when this will be released?
The last update of the npm package was 7 months ago ^^

@lb-
Copy link
Contributor

lb- commented May 13, 2022

Running into this issue also - looking forward to seeing this released 👍

@alexander-schranz
Copy link
Contributor

alexander-schranz commented May 13, 2022

I don't know when this is going to release. But currently you could add a abstract controller in your project which you extend like here: https://discuss.hotwired.dev/t/stimulus-and-typescript/2458/3 and when released you can remove it and replace in your controllers with the stimulus controller again.

@lb-
Copy link
Contributor

lb- commented May 13, 2022

Thanks for that. I did go down that path but alas our project is quite strict and does not allow ts ignore comments.

For now we are in assessment mode for stimulus so adding declarations where needed will have to suffice.

Just curious - do you know why the original definition uses Element and not HTMLElement | SVGElement (apologies if that is a TS newbie question).

@rik
Copy link
Contributor Author

rik commented May 13, 2022

As far as I can tell, Element is equivalent to HTMLElement | SVGElement because those are the only two subclasses of Element.

@lb-
Copy link
Contributor

lb- commented May 13, 2022

As far as I can tell, Element is equivalent to HTMLElement | SVGElement because those are the only two subclasses of Element.

But then I run into this. https://stackoverflow.com/a/58773720/8070948

Anyway. Probably de-railing this issue's chat - I'll just ask on the community thread if I get more stuck here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants