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

feat(dialog): Dialogs can define the type for their dialog data #23985

Open
Airblader opened this issue Nov 18, 2021 · 4 comments
Open

feat(dialog): Dialogs can define the type for their dialog data #23985

Airblader opened this issue Nov 18, 2021 · 4 comments
Labels
area: material/dialog feature This issue represents a new feature or feature request rather than a bug or bug fix needs: discussion Further discussion with the team is needed before proceeding P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@Airblader
Copy link
Contributor

Airblader commented Nov 18, 2021

Feature Description

#4398 introduced a generic type argument for MatDialogConfig. This type argument is forwarded in open. However, the type is never tied in anyway to the actual component, making the type argument an essentially useless type assertion.

While due to the nature of Angular's dependency injection full type-safety cannot be achieved, it would be nice if at least the dialog component could declare the type it receives itself in such a way that a call to open will infer the correct type automatically. In order to achieve this, the dialog component needs to somehow expose this information. Here are a few possible solutions in no particular order, though others might exist:

Interface with type inference

We could define an interface and a helper type as follows:

interface MatDialogHasData<T> {
  data: T;
}

type ExtractDialogDataType<T> = T extends MatDialogHasData<infer D> ? D : any;

We can then adjust the type signature of open to infer the type accordingly:

open<T, R = any>(
  component: ComponentType<T>, 
  data?: MatDialogConfig<ExtractDialogDataType<T>>
): MatDialogReg<T, R>

Finally, a dialog component would be required to have a public field named data of the according type, which would typically be achieved through the injection itself:

constructor(@Inject(MAT_DIALOG_DATA) public data: MyDialogDataType) {}

Note: This change is breaking because the generic signature of open changes and because dialog components could have an unrelated data field which would lead to an unintended type inference. Unfortunately we cannot prevent this here due to TypeScript's structural typing nature. In particular users would no longer be able to manually define the generic type argument. However see further below for a solution to this problem.

Interface with explicit method

Instead, we could also introduce an interface with an explicit method to be implemented:

interface MatDialogHasData<T> {
  getDialogData(): T
}

Otherwise the approach is largely similar. This change is only breaking in the generic type signature (I'm not sure whether you count this as a breaking change), however we could actually avoid that by changing our helper type to

type ExtractDialogDataType<T, FALLBACK> = T extends MatDialogHasData<infer D> ? D : FALLBACK;

and then using the type argument as a fallback:

open<T, D = any, R = any>(
  component: ComponentType<T>, 
  data?: MatDialogConfig<ExtractDialogDataType<T, D>>
): MatDialogReg<T, R>

Note: The same could be done in the solution above. I believe we could make this a completely backwards-compatible change, excluding of course the exotic situation that a component happens to already have a getDialogData method for unrelated reasons. The only disadvantage here is forcing the component to provide a public method. In practice I don't consider this a concern as Angular already forces us to make many things public that shouldn't be public from a component API perspective.


On a very similar note, it would be nice to have the same for the inferred result type R, i.e. the component being able to define the type of data it can return, and have this information flow through the open method to the caller. The approaches mentioned above could, in theory, cover this as well.

Use Case

Given some MyDialogComponent which expects dialog data of type MyDialogData, I would like

open(MyDialogComponent, {
  data: {
    // …
  }
});

to type-check data against MyDialogData without having to manually specify open's type arguments, but rather have this information flow from MyDialogComponent.

@Airblader Airblader added feature This issue represents a new feature or feature request rather than a bug or bug fix needs triage This issue needs to be triaged by the team labels Nov 18, 2021
@jelbourn jelbourn added area: material/dialog needs: discussion Further discussion with the team is needed before proceeding P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Nov 18, 2021
@angular-robot
Copy link
Contributor

angular-robot bot commented Mar 14, 2022

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@vampyr09
Copy link

vampyr09 commented Mar 28, 2022

I very much like the idea.
Where I work we "solved" this issue with a custom marker type MatDialogInput<T> to not depend on the property name of the field ("data").

// branded input so it could be pulled through the constructor parameters
export type MatDialogInput<TInput> = {
  _brand: 'MatDialogInput' |  undefined;
} & TInput;

It is used like this:

constructor(@Inject(MAT_DIALOG_DATA) public data: MatDialogInput<DataType>) {}

and then we pull the inner marker type out of all the constructor parameters.

We grap the return type of the dialog in the very same way (MatDialogRef<D, R>).

In the end our implementation looks like this:

class TypedMatDialog {
  open<TComponent, 
       TInput extends FindMatDialogInput<TComponent>, 
       TOutput extends FindMatDialogOutput<TComponent>
      >(
        component: TComponent, 
        ...data: [TInput] extends [never] 
            ? [] | [MatDialogConfig] 
            : [Omit<MatDialogConfig, 'data'> & { data: TInput }]
       ): MatDialogRef<TComponent, TOutput> {
    return this.dialog.open(component, ...config);
  }
}

One further pro of this solution is that it should be backwards compatible because if the marker type is not used, then the type could still be inferred as any.

@angular-robot
Copy link
Contributor

angular-robot bot commented Apr 3, 2022

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

@Harpush
Copy link

Harpush commented Aug 22, 2022

I really like this idea. It will allow to just pass the component and get type safety for data and result. Even better if the dialog has no data or no result this can be type checked too.
One thing I think is a good addition too is allowing the dialog itself to somehow define the config - after all the dialog knows what's the best dimensions for it rather than the one opening it. But that might be another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: material/dialog feature This issue represents a new feature or feature request rather than a bug or bug fix needs: discussion Further discussion with the team is needed before proceeding P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
5 participants