feat(data-table): allow dynamic data source #5093
Conversation
googlebot
added
the
cla: yes
| @Input() | ||
| get dataSource(): DataSource<T> { return this._dataSource; } | ||
| set dataSource(dataSource: DataSource<T>) { | ||
| if (this._dataSource == dataSource) { return; } |
| } | ||
|
|
||
| ngOnDestroy() { | ||
| // TODO(andrewseguin): Disconnect from the data source so | ||
| // that it can unsubscribe from its streams. | ||
| this._onDestroy.next(); |
There was a problem hiding this comment.
I believe you want to complete the subject so it won't emit anymore and the datasource listener will also finish.
this._onDestroy.complete();
There was a problem hiding this comment.
I believe it goes either way, according to this documentation: If this second Observable emits an item or sends a termination notification, the Observable returned by TakeUntil stops mirroring the source Observable and terminates.
Though I do agree that complete() looks nicer and more definitive, will change to that.
| } | ||
|
|
||
| /** | ||
| * Create the embedded view for the header template and place it in the header row view container. | ||
| */ | ||
| renderHeaderRow() { | ||
| const cells = this.getHeaderCellTemplatesForRow(this._headerDefinition); | ||
| _renderHeaderRow() { |
There was a problem hiding this comment.
Can this be marked private? Or is it used somewhere else? Ditto to some of the other methods in this class.
| get dataSource(): DataSource<T> { return this._dataSource; } | ||
| set dataSource(dataSource: DataSource<T>) { | ||
| if (this._dataSource == dataSource) { return; } | ||
| this._data = []; |
There was a problem hiding this comment.
Can you move most of this setter out into a separate function like _updateDataSource or _switchDataSource?
| // Re-render the rows if any of their columns change. | ||
| // TODO(andrewseguin): Determine how to only re-render the rows that have their columns changed. | ||
| const rowColumnsChange = | ||
| this._rowDefinitions.toArray().map((rowDef: BaseRowDef) => rowDef.columnsChange); |
There was a problem hiding this comment.
QueryList has a map function so you don't need toArray
https://angular.io/api/core/QueryList
You also shouldn't need a type in the map function since it is given by the generic
There was a problem hiding this comment.
Good catch, thanks
| @@ -161,7 +145,7 @@ export class CdkTable<T> implements CollectionViewer { | |||
| } | |||
|
|
|||
| ngOnDestroy() { | |||
| this._onDestroy.next(); | |||
| this._onDestroy.complete(); | |||
There was a problem hiding this comment.
I think next() is still required for takeUntil(example)
I suspect @ErinCoughlan was suggesting you use both
There was a problem hiding this comment.
Thanks for the heads up, I appreciate the plunker to help me understand. Looking at the RxJS code does indicate that it doesn't look to the completion event.
Returns the values from the source observable sequence until the other observable sequence produces a value.
This doesn't line up with the documentation from reactivex.io which says that either an emitted value or termination notification would end it.
Will change it to be next() and complete(). Not totally sure the complete() is necessary but we can discuss another time.
There was a problem hiding this comment.
Looks like the rxjs docs will eventually be updated to reflect behavior being inconsistent with reactivex.io ReactiveX/rxjs#2160
Also don't think complete() is required, but as far as I've seen, this SO answer has been the recommended approach.
There was a problem hiding this comment.
SGTM, thanks for the context. We'll have to continue looking at this for other components where we want to unsubscribe on destroy
jelbourn
added
pr: lgtm
action: merge
andrewseguin
force-pushed
the
table-data-source-setter
branch
from
e994c2a
to
454633b
Compare
|
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. |
No description provided.