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.

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

Jump to bottom

build(table): explicitly define MatTable ctor #10982

Merged
merged 2 commits into from
Apr 25, 2018
Merged

build(table): explicitly define MatTable ctor #10982

merged 2 commits into from
Apr 25, 2018

Conversation

andrewseguin
Copy link
Contributor

Explicitly define the ctor due to a tsickle issue which is depended on by compiler-cli.

Fixes #9329

@andrewseguin andrewseguin requested a review from jelbourn April 24, 2018 20:46
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 24, 2018
jelbourn
jelbourn approved these changes Apr 24, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Is this necessary for 6.0?

src/lib/table/table.ts Outdated
@@ -24,4 +32,15 @@ import {CDK_TABLE_TEMPLATE, CdkTable} from '@angular/cdk/table';
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class MatTable<T> extends CdkTable<T> { }
export class MatTable<T> extends CdkTable<T> {
// TODO(andrewseguin): Remove this explicitly set ctor when the compiler knows how to properly
Copy link
Member

Choose a reason for hiding this comment

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

Prefer constructor to ctor

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs review labels Apr 24, 2018
@andrewseguin
Copy link
Contributor Author

It's not necessary for 6.0 but would be nice to have in sooner than later. However as of right now we are distributing a broken build of table.js for all ES6 users.

https://unpkg.com/@angular/material@5.2.5/esm2015/table.js

/** @nocollapse */
MatTable.ctorParameters = () => [];

@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Apr 24, 2018
@jelbourn
Copy link
Member

Marking as P1, then.

@andrewseguin andrewseguin merged commit 848fe58 into angular:master Apr 25, 2018
@andrewseguin andrewseguin mentioned this pull request Apr 26, 2018
Closed
3 tasks
@VangelisAlSghir
Copy link

Can we expect this to be in RC.15? We really need to use Material Tables targeting ES6.

@andrewseguin
Copy link
Contributor Author

@VangelisAlSghir Yes, it should be fixed as of 6.0.0-rc.14 (released a few days ago)

@VangelisAlSghir
Copy link

Are you sure? I just made a new angular project and targeted ES6 and tried to use a material table with the example code from https://github.com/angular/material2/blob/6.0.0-rc.14/src/demo-app/table/mat-table-data-source/mat-table-data-source.html but I still get following errors in the console:

TypeError: Cannot read property 'find' of undefined
at MatHeaderRowDef.ngOnChanges (webpack-internal:///./node_modules/@angular/cdk/esm2015/table.js:76)

and

ypeError: Cannot read property 'diff' of undefined
at MatRowDef.getColumnsDiff (webpack-internal:///./node_modules/@angular/cdk/esm2015/table.js:86)

@andrewseguin
Copy link
Contributor Author

Ah bummer, looks like the same "fix" needs to be applied to those classes as well. Will be able to make a change next week but it'll like be a part of v6.0.1 or v6.0.2

@VangelisAlSghir
Copy link

The sooner the better for us :)

@andrewseguin andrewseguin mentioned this pull request May 9, 2018
Merged
@andrewseguin
Copy link
Contributor Author

PR has been created, hoping it'll be merged this week and go out with our next release on Monday. Thanks for your patience and persistence with the issue

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Table] Does not work when targetting ES6
4 participants
@andrewseguin @jelbourn @VangelisAlSghir @googlebot