taiga-family / taiga-ui

Angular UI Kit and components library for awesome people
https://taiga-ui.dev
Apache License 2.0
3.25k stars 453 forks source link

🚀 - `TuiDialogService.open()` should be properly typed #7617

Open hakimio opened 5 months ago

hakimio commented 5 months ago

Playground Link

No response

Description

Try to supply some data to dialog.open() method.

this.dialogService
    .open<MyOutputType>(new PolymorpheusComponent(MyComponent), {
        label: 'Star wars. Episode III',
        size: 's',
        // "data" here has type "any"
        data: {button: 'Do it!'},
    })
    // "result" here has proper typing
    .subscribe(result => this.myService.process(result));

Notice how dialog data is typed as any and there is no obvious way to set correct type. The only way to fix the data type is to use the following ugly workaround:

private dialogService: AbstractTuiDialogService<TuiDialogOptions<MyDataType>> = inject(TuiDialogService);

The open() method should accept a second generic to properly type dialog data.

Angular version

17.3.10

Taiga UI version

3.81.0

Which browsers have you used?

Which operating systems have you used?

splincode commented 5 months ago

maybe related:

https://github.com/taiga-family/taiga-ui/pull/6175/files https://github.com/taiga-family/taiga-ui/pull/6115/files

hakimio commented 5 months ago

@splincode the mentioned PR doesn't fix the issue. We need second generic on the open() method to type data config, not a way to pass any Angular component to open(). I am fine with just passing PolymorpheusComponent.

EDIT: updated the example in the issue description.

waterplea commented 5 months ago

For now you can type options as a const:

const options: TuiDialogOptions<{ button: string }> = {
  label: 'Star wars. Episode III',
  size: 's',
  data: {button: 'Do it!'},
}

this.dialogService
    .open<MyOutputType>(new PolymorpheusComponent(MyComponent), options)
    .subscribe(result => this.myService.process(result));

In theory we wanted to extract data type from content, because it has typed context, at least in case of templates and functions, but so far I haven't been able to figure that out fast enough and backlogged it.

hakimio commented 5 months ago

Your workaround doesn't seem to work:

TS error

waterplea commented 5 months ago

Sorry, my mistake. Partial<TuiDialogOptions<...>>

hakimio commented 5 months ago

Partial<TuiDialogOptions<MyDataType>> is almost as long as AbstractTuiDialogService<TuiDialogOptions<MyDataType>> and not that much better workaround. open() definitely needs a second generic and, honestly, I don't see how that can break anything.

splincode commented 5 months ago

@hakimio do you mean about second generic like that https://github.com/taiga-family/taiga-ui/pull/6175/files ?

hakimio commented 5 months ago

Almost, just instead of this.dialogs.open<TuiDayRange, TuiDialogOptions<TuiMobileCalendarData>>(), I would like to simply use this.dialogs.open<TuiDayRange, TuiMobileCalendarData>(). Basically, I don't think TuiDialogOptions should be mandatory. Anyway, it could be a good start.

hakimio commented 5 months ago

Maybe DialogService could override the open() method from the abstract class to set different generics.

splincode commented 5 months ago

@hakimio what do you think? https://github.com/taiga-family/taiga-ui/pull/7621

hakimio commented 5 months ago

Not a fan, to be honest.

Ideally, DialogService class generic could look like this:

export class TuiDialogService<T = any> extends TuiPopoverService<TuiDialogOptions<T>> {}

And DialogService should override it's open() method. Sth like this:

    // "T" generic here comes from the class definition
    public override open<G = void, V extends T = T>(
        content: PolymorpheusContent<<TuiDialogOptions<V> & TuiPopoverContext<G>>,
        options: Partial<TuiDialogOptions<V>> = {},
    )

Sth similar could be done for MobileDialogService where open() is already overriden. I don't think you should change the open() method in the base PopoverService.

splincode commented 5 months ago

@hakimio I think, you can make contribution PR

hakimio commented 5 months ago

Only if @waterplea is willing to accept the changes I mentioned.

waterplea commented 5 months ago

At the moment I'm focusing on 4.0 and unable to properly assess the suggestion. From the brief look, generic in class definition makes no sense since this generic is relative to open method only and a particular content you pass to it, that expects particular data type. That's why ideally it would be inferred from the content passed. I'll get back to it when we finish first release candidate. In the meantime you can define a type if you dislike long workaround:

export interface WithData<T> extends Partial<TuiDialogOptions<T>> {}

And define options as const like I suggested earlier.