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

MatDialog open method R generic is too loose #12898

Open
jelbourn opened this issue Aug 29, 2018 · 6 comments
Open

MatDialog open method R generic is too loose #12898

jelbourn opened this issue Aug 29, 2018 · 6 comments
Labels
area: material/dialog P4 A relatively minor issue that is not relevant to core functions refactoring This issue is related to a refactoring

Comments

@jelbourn
Copy link
Member

Because the R generic on MatDialog.open isn't constrained by arguments to the function, it's effectively always any. We should remove the method generic and change the return generic to {}, which will force people to explicitly state which type it is when consuming it.

@jelbourn jelbourn added refactoring This issue is related to a refactoring P4 A relatively minor issue that is not relevant to core functions labels Aug 29, 2018
@crisbeto
Copy link
Member

The reason R defaults to any was to avoid breaking changes at the time we introduced the argument.

@rkirov
Copy link
Contributor

rkirov commented Aug 30, 2018

The issue is not with 'any', but with allowing R to be unsafely inferred to be anything. For example, the following always compiles.

let ref: MatDialogRef<T, SomeMadeUpType> = open(...);

It looks like any other code where the function call returned some type, but the developer choose to write it out for clarity.

By forcing the developer to write let ref = open(...) as MatDialogRef<T, SomeMadeUpType>; it becomes clear that this is an unsafe cast.

@crisbeto crisbeto self-assigned this Sep 6, 2018
@crisbeto crisbeto added the has pr label Sep 6, 2018
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 6, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 19, 2018
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 8, 2019
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jul 9, 2019
Removes the `R` generic, because it always ends up as `any` in the result subscriptions.

Fixes angular#12898.
@JWess
Copy link

JWess commented Mar 7, 2022

It would be nice if the type of R could be inferred from the type of Component being passed into open().

@mmalerba mmalerba removed the has pr label Dec 8, 2022
@aceArt-GmbH
Copy link

The only type safe thing would be if the dialog and it's return value would be passed around.
Currently the dialog caller has to declare some return value and some input variables and the dialog does the same thing in reverse (declaring input types and having output variables)

@Samuel-Therrien-Beslogic

Whilst I'm not entirely certain if the best default should be unknown, {} or void. I did notice that as well and since then I am using patch-package to replace in node_modules/@angular/material/dialog/index.d.ts

-export declare class MatDialogRef<T, R = any> {
+export declare class MatDialogRef<T, R = void> {

@aceArt-GmbH
Copy link

Also check out a proposed workaround for a similar type issue #24538 (comment).
Maybe it can be made official and fix both issues at once?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: material/dialog P4 A relatively minor issue that is not relevant to core functions refactoring This issue is related to a refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants