Skip to content

build(table): explicitly define MatTable ctor - #10982

Merged
andrewseguin merged 2 commits into
angular:masterfrom
andrewseguin:table-fix-es6
Apr 25, 2018
Merged

andrewseguin merged 2 commits into
angular:masterfrom
andrewseguin:table-fix-es6

Conversation

@andrewseguin

Copy link
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread src/lib/table/table.ts Outdated
})
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

Marking as P1, then.

@andrewseguin
andrewseguin merged commit 848fe58 into angular:master Apr 25, 2018
@VangelisAlSghir

Copy link
Copy Markdown

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

@andrewseguin

Copy link
Copy Markdown
Contributor Author

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

@VangelisAlSghir

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

The sooner the better for us :)

@andrewseguin

Copy link
Copy Markdown
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
Copy Markdown

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.

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

Development

Successfully merging this pull request may close these issues.

[Table] Does not work when targetting ES6

4 participants