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
fix(cdk/layout): support readonly arrays for public methods #20252
Conversation
724defb
to
6695d3b
Compare
return stateObservable.pipe(map(breakpointStates => { | ||
return breakpointStates.reduce((previousState, {matches, query}) => ({ | ||
...previousState, | ||
breakpoints: {...previousState.breakpoints, [query]: matches}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is shorter, but less efficient since it transpiles to two Object.assign
calls. I'm guessing that's why it was previously written with a forEach
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, reverted.
@@ -71,7 +71,7 @@ export class BreakpointObserver implements OnDestroy { | |||
* @param value One or more media queries to check. | |||
* @returns A stream of matches for the given queries. | |||
*/ | |||
observe(value: string | string[]): Observable<BreakpointState> { | |||
observe(value: string | readonly string[]): Observable<BreakpointState> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ReadonlyArray<string>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReadonlyArray<string>
and readonly string[]
have exactly the same meaning. I can adjust if the ReadonlyArray<>
style is more adopted in the repo.
PS: It would be nice to pick one of the styles and use it consistently. In angular repo someone created an issue relating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer readonly string[]
myself
64e99ab
to
d7e262c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -71,7 +71,7 @@ export class BreakpointObserver implements OnDestroy { | |||
* @param value One or more media queries to check. | |||
* @returns A stream of matches for the given queries. | |||
*/ | |||
observe(value: string | string[]): Observable<BreakpointState> { | |||
observe(value: string | readonly string[]): Observable<BreakpointState> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer readonly string[]
myself
* fix(cdk/layout): support readonly arrays for public methods * refactor(cdk/layout): remove unnecessary type annotations (cherry picked from commit e3699dc)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This PR add support for
readonly string[]
for methodsisMatched
andobserve
as they doesn't accept it currently.Also, I've refactored it a bit to remove some unnecessary type annotations.