search

angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.32k stars 6.72k forks source link

bug(MatExpansionPanel): accordion property should be nullable #20152

Open dev054 opened 4 years ago

dev054 commented 4 years ago

Reproduction

https://stackblitz.com/edit/angular-jxhwbd

Steps to reproduce:

  1. Try to access an accordion property from an expansion panel without accordion parent.

Expected Behavior

accordion should be MatAccordionBase | null to reflect its definition.

Actual Behavior

accordion is defined as:

https://github.com/angular/components/blob/3a6c16f0c04ce8784b0e015a71a889864f51a5f4/src/material/expansion/expansion-panel.ts#L134

... while it's optional in fact, as the expansion-panel can be used without accordion (btw the constructor parameter should also be marked with "?"):

https://github.com/angular/components/blob/3a6c16f0c04ce8784b0e015a71a889864f51a5f4/src/material/expansion/expansion-panel.ts#L151

--

PS: Note that there are other places where the constructor parameter is decorated with @Optional, but the property itself isn't. Maybe a good candidate for a rule to prevent this mistake.

Environment

Angular CLI: 10.0.3 Node: 13.7.0 OS: darwin x64

Angular: 10.0.4 ... animations, common, compiler, compiler-cli, core, forms ... language-service, localize, platform-browser ... platform-browser-dynamic, router Ivy Workspace: Yes

Package Version

@angular-devkit/architect 0.1000.3 @angular-devkit/build-angular 0.1000.3 @angular-devkit/build-optimizer 0.1000.3 @angular-devkit/build-webpack 0.1000.3 @angular-devkit/core 10.0.3 @angular-devkit/schematics 10.0.3 @angular/cdk 10.1.0 @angular/cli 10.0.3 @angular/flex-layout 10.0.0-beta.32 @angular/material 10.1.0 @ngtools/webpack 10.0.3 @schematics/angular 10.0.3 @schematics/update 0.1000.3 rxjs 6.6.0 typescript 3.9.7 webpack 4.43.0

crisbeto commented 4 years ago

For context around marking @Optional parameters as optional in the type, I looked into doing this a long time ago, but the problem is that optional parameters have to be last in the constructor and there are plenty of places where that's not the case at the moment. If we decided to do it now, we'd have to introduce a lot of breaking constructor changes.

Technically the correct approach would be to mark them as | null since that's what Angular will return if the token doesn't match anything, but we can't do that right now either, because ViewEngine throws an error for it. We may be able to do it once ViewEngine is removed.

dev054 commented 4 years ago

Got it, thanks for the fast response! While you can't do it right now due to the how ViewEngine works, could you start adding @deprecated and @breaking-change like you generally do for these cases? Thus, when the change is possible, it will be much easier, as the deprecation time would be respected. What do you think?