Skip to content

fix(material/table): return undefined sort and paginator #31593

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(material/table): return undefined sort and paginator #31593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions goldens/material/table/index.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ export class MatTableDataSource<T, P extends MatPaginator = MatPaginator> extend
filterPredicate: (data: T, filter: string) => boolean;
_orderData(data: T[]): T[];
_pageData(data: T[]): T[];
get paginator(): P | null;
get paginator(): P | null | undefined;
set paginator(paginator: P | null | undefined);
_renderChangesSubscription: Subscription | null;
get sort(): MatSort | null;
get sort(): MatSort | null | undefined;
set sort(sort: MatSort | null | undefined);
sortData: (data: T[], sort: MatSort) => T[];
sortingDataAccessor: (data: T, sortHeaderId: string) => string | number;
Expand Down
16 changes: 6 additions & 10 deletions src/material/table/table-data-source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,16 @@ export class MatTableDataSource<T, P extends MatPaginator = MatPaginator> extend
* Instance of the MatSort directive used by the table to control its sorting. Sort changes
* emitted by the MatSort will trigger an update to the table's rendered data.
*/
get sort(): MatSort | null {
get sort(): MatSort | null | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a public API change. It might be better to change the setter to || null so the runtime matches the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got LGTM from @andrewseguin to proceed in this direction. I already prepared google3 for that. It was easy - 3 places where the type was explicitly copied.

Given that it always could have returned undefined, there may be code handling that. A breaking compile time check is safer than changing runtime behavior. Coalescing to null caught tests in google3 that expected that what they put in is what they get back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change aligns closer to what is actually happening, since sort actually can return undefined when its unset. Since this expands the set rather than constricting, I don't know if this counts as a major "breaking" change.

return this._sort;
}

set sort(sort: MatSort | null | undefined) {
// Treat undefined like the initial this._sort value.
// Note that the API can be changed in a breaking change to fix the cast.
this._sort = sort as MatSort | null;
this._sort = sort;
this._updateChangeSubscription();
}

private _sort: MatSort | null;
private _sort: MatSort | null | undefined;

/**
* Instance of the paginator component used by the table to control what page of the data is
Expand All @@ -126,18 +124,16 @@ export class MatTableDataSource<T, P extends MatPaginator = MatPaginator> extend
* e.g. `[pageLength]=100` or `[pageIndex]=1`, then be sure that the paginator's view has been
* initialized before assigning it to this data source.
*/
get paginator(): P | null {
get paginator(): P | null | undefined {
return this._paginator;
}

set paginator(paginator: P | null | undefined) {
// Treat undefined like the initial this._paginator value.
// Note that the API can be changed in a breaking change to fix the cast.
this._paginator = paginator as P | null;
this._paginator = paginator;
this._updateChangeSubscription();
}

private _paginator: P | null;
private _paginator: P | null | undefined;

/**
* Data accessor function that is used for accessing data properties for sorting through
Expand Down
Loading