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.

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

build: remove es2015 workaround #14066

Merged

Conversation

devversion
Copy link
Member

With two commits (2adced1 and a22a9fa), we recently tried to add a workaround for an Angular bug that causes Angular Material to not work w/ ES2015.

The workaround technically worked but had to be partially reverted because Google Closure compiler was no longer able to remove unused classes (due to a function with side-effects that modified the actual component metadata).

Since the workaround (in the state it is right now) is not helping at all and a fix will land with Angular v7.1.0 (angular/angular@95743e3), we are removing the workaround completely.

Closes #12760

With two commits (2adced1 and a22a9fa), we recently tried to add a workaround for an Angular bug that causes Angular Material to not work w/ ES2015. The workaround technically worked but had to be partially reverted because Google Closure compiler was no longer able to remove unused classes (due to a function with side-effects that modified the actual component metadata). Since the workaround (in the state it is right now) is not helping at all and a fix will land with Angular `v7.1.0` (angular/angular@95743e3)

Closes angular#12760
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 9, 2018
@devversion devversion added the target: patch This PR is targeted for the next patch release label Nov 9, 2018
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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Nov 9, 2018
@vivian-hu-zz vivian-hu-zz merged commit 21e646a into angular:master Nov 10, 2018
@devversion devversion deleted the build/remove-es2015-workaround branch November 10, 2018 10:30
vivian-hu-zz pushed a commit that referenced this pull request Nov 12, 2018
With two commits (2adced1 and a22a9fa), we recently tried to add a workaround for an Angular bug that causes Angular Material to not work w/ ES2015. The workaround technically worked but had to be partially reverted because Google Closure compiler was no longer able to remove unused classes (due to a function with side-effects that modified the actual component metadata). Since the workaround (in the state it is right now) is not helping at all and a fix will land with Angular `v7.1.0` (angular/angular@95743e3)

Closes #12760
@jmpreston
Copy link

NOT FIXED in 7.1.0. TS 3.1.6. See issue #14100

@devversion
Copy link
Member Author

devversion commented Nov 22, 2018

@svstartuplab As mentioned in the PR description. This is not a problem from the Angular Material side, so we cannot do much about it. Based on angular/angular@95743e3, this issue should be fixed if you use Angular Material 7.1.0 and Angular 7.1.0

@jmpreston
Copy link

I just upgraded to 7.1.0 from 6 a few days ago for Angular and Material and the problem started. I think I'm using the latest versions of everything.

@devversion
Copy link
Member Author

devversion commented Nov 22, 2018

Angular Material v7.1.0 just came out yesterday. Can you please retry? Thanks!

Also making sure that you are using Angular 7.1.0 as well.

@jmpreston
Copy link

As mentioned, I am using 7.1.0. From npm view:

@angular/material@7.1.0 | MIT | deps: 1 | versions: 101
Angular Material
https://github.com/angular/material2#readme

keywords: angular, material, material design, components

dist
.tarball: https://registry.npmjs.org/@angular/material/-/material-7.1.0.tgz
.shasum: 441f2d03a97a1fa35546b61dcde86db34f0f5efc
.integrity: sha512-bgotNpSfGLjNZ1AcTyhs6XS7trF4I7UHwQmfa0l8y3Gf9plwErPDfQe2XqnayRyG9nTwHj9f1lQ45X5mr3/0/A==
.unpackedSize: 20.4 MB

dependencies:
tslib: ^1.7.1 

maintainers:
- angular <devops+npm@angular.io>

dist-tags:
latest: 7.1.0     next: 7.0.0-rc.2 

@devversion
Copy link
Member Author

devversion commented Nov 22, 2018

Okay, assuming that you use Angular v7.1.0 and Angular Material v7.1.0, what's your exact exception?

In our code, the ctorParameters workaround is completely gone, so I'm wondering what your exception looks like?

@devversion
Copy link
Member Author

If your exception is Uncaught TypeError: ctorParameters.map is not a function, then this means that you somehow still use an old version.

Otherwise if there is a different exception, then this is an Angular issue that should be reported on here

@jmpreston
Copy link

  1. The above info comes from: npm view @angular/material

  2. import { MatTableModule } from '@angular/material/table';

I'm no expert but I seem to be using the latest version.

  1. npm view @angular/material/table version === 0.0.1

@devversion
Copy link
Member Author

devversion commented Nov 22, 2018

npm view @angular/material checks the online registry, so that's not necessarily the version you have installed. Can you please do the following:

  1. Run npm ls @angular/core and post output here
  2. Run npm ls @angular/material and post output here
  3. Write the error message here in the issue.

Thanks.

@jmpreston
Copy link

jmpreston commented Nov 22, 2018

@angular/core@7.0.3
@angular/material@7.0.3

From package.json:

"@angular/core": "^7.0.3",
"@angular/material": "^7.0.3",

So something happened in the upgrade and it updated the registry but not the packages?

@devversion
Copy link
Member Author

devversion commented Nov 22, 2018

Yeah, so that shows that you didn't update Angular and Angular Material to the latest version. Do you have a lock file? probably run npm update (https://docs.npmjs.com/cli/update)

Sorry, I cannot help more with the issue. Please consider asking on Stackoverflow. Thanks for the feedback though.

@jmpreston
Copy link

Thank you. I did use both npm install and npm update. Just another computer mystery :-)

@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 10, 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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency injection not working for derived classes in ES2015
5 participants