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

ng cli build --prod fails when activating noUnusedParameters in tsconfig.json #4443

Closed
reppners opened this issue May 9, 2017 · 11 comments · Fixed by #4946
Closed

ng cli build --prod fails when activating noUnusedParameters in tsconfig.json #4443

reppners opened this issue May 9, 2017 · 11 comments · Fixed by #4946
Assignees
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix P5 The team acknowledges the request but does not plan to address it, it remains open for discussion refactoring This issue is related to a refactoring

Comments

@reppners
Copy link

reppners commented May 9, 2017

Bug, feature request, or proposal:

Bug/Improvement

What is the expected behavior?

Should be able to build with ng build --prod when having compilerOptions.noUnusedParameters: true in tsconfig.json

What is the current behavior?

Build fails with

ERROR in my-project/src/$$_gendir/node_modules/@angular/material/typings/index.ngfactory.ts (2252,41): 'l' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdButton.html (8,1): 'v' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdAnchor.html (8,1): 'v' is declared but never used.

ERROR in my-project/src/$$_gendir/node_modules/@angular/material/typings/index.ngfactory.ts (3345,31): 'l' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdInputContainer_Host.html (2,1): '$event' is declared but never used.

ERROR in my-project/src/$$_gendir/node_modules/@angular/material/typings/index.ngfactory.ts (4539,31): 'l' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdListItem_Host.html (2,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdSelect.html (23,5): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdSelect.html (2,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdSelect.html (15,7): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdSidenavContainer.html (2,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdTabGroup.html (5,3): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdTabHeader.html (2,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdTabHeader.html (12,3): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdTabHeader.html (20,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdButtonToggle.html (17,1): 'v' is declared but never used.

ERROR in my-project/src/$$_gendir/node_modules/@angular/material/typings/index.ngfactory.ts (7426,31): 'l' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.MdSlideToggle_Host.html (2,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.SimpleSnackBar.html (3,1): '$event' is declared but never used.

ERROR in my-project/node_modules/@angular/material/typings/index.d.ts.TooltipComponent_Host.html (2,1): '$event' is declared but never used.

What are the steps to reproduce?

  • create fresh project ng new my-project
  • install and include @angular/material as per the docs
  • set "noUnusedParameters": true in the projects tsconfig.json
  • run ng build --prod

What is the use-case or motivation for changing an existing behavior?

Apps using material should be able to compile with noUnusedParameters: true in tsconfig.json.

Which versions of Angular, Material, OS, browsers are affected?

"@angular/animations": "^4.1.0",
"@angular/common": "^4.0.0",
"@angular/compiler": "^4.0.0",
"@angular/core": "^4.0.0",
"@angular/material": "^2.0.0-beta.3",
"@angular/cli": "1.0.1",

Is there anything else we should know?

Looks like this is some generated code from html templates. I don't know if it's even the fault of angular material.

The fix would be to define parameters with a leading _. See microsoft/TypeScript#9860.

@devversion
Copy link
Member

Yeah at some point when TSlint deprecated the no-unused-variable rule I tried to use that option too.

Unfortunately the compiler option doesn't seem to be very smart and flexible, so I didn't move ahead with that.

In theory we could prefix all those variables with an underscore, but I'm not sure if TypeScript improves their option at some point & if it's worth the efforts.

@reppners
Copy link
Author

Seems like the unused parameter issue also got some attention in the angular core recently angular/angular#15532

Would this solve this issue also or is it just fixing the index.ngfactory.ts?

@jelbourn jelbourn added feature This issue represents a new feature or feature request rather than a bug or bug fix P5 The team acknowledges the request but does not plan to address it, it remains open for discussion refactoring This issue is related to a refactoring labels May 10, 2017
@devversion
Copy link
Member

@jelbourn Are you thinking we should prefix those variables with an underscore? And probably compile the AOT demo-app with noUnusedParameters and noUnusedLocals.

If I recall correctly there will be a lot of locals and parameters that need to be prefixed with an underscore.

@jelbourn
Copy link
Member

  1. There's nothing we can do about unused parameters in the generated code; that's all on Angular
  2. If we have any unused parameters in our own source, we should just remove them.

Other than those two categories I don't think anything should change.

@devversion
Copy link
Member

If we enable noUnusedParameters Material shows a lot of errors for stuff like that:

// Value is not used. Needs to be prefixed
_onChange = (value: any) => {};

/**
 * Sets the vertical placement of the tile in the list.
 * This method will be implemented by each type of TileStyler.
 * @docs-private
 */
setRowStyles(tile: MdGridTile, rowIndex: number, percentWidth: number, gutterWidth: number) {}

The setRowStyles function is not implemented and is kind of "abstract" (but other functions are implemented).

TypeScript would force us to prefix them with an underscore. Not a big deal, but just want to let you know what will change.

@jelbourn
Copy link
Member

Function params should not be prefixed with an underscore.

If a method is abstract, it should be marked as abstract.

For that onChange, it should be

_onChange: (any?) => void = () => {};

@devversion
Copy link
Member

Yeah that makes sense. I didn't write the grid-list so I don't know why it isn't made abstract.

Most of those noUnusedParameters failures can be fixed easily. But I think we need a way to check this on the CI (once we fixed all of those)

@jelbourn
Copy link
Member

jelbourn commented May 11, 2017

Yeah, if the generated code doesn't have an issue then a CI check would be necessary.

@devversion devversion self-assigned this May 11, 2017
@wulfsberg
Copy link

I think this is considerably more than a "nice to have". A library like Material should not block people from using strict compiler checks in their projects.

@jelbourn
Copy link
Member

That priority level mainly comes from the fact that Angular's generated code is the culprit in most cases, which is outside the scope of material.

devversion added a commit to devversion/material2 that referenced this issue Jun 3, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes angular#4443
devversion added a commit to devversion/material2 that referenced this issue Jun 3, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes angular#4443
devversion added a commit to devversion/material2 that referenced this issue Jun 8, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes angular#4443
devversion added a commit to devversion/material2 that referenced this issue Jun 8, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes angular#4443
devversion added a commit to devversion/material2 that referenced this issue Jun 8, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes angular#4443
andrewseguin pushed a commit that referenced this issue Jun 8, 2017
* Recently Angular core fixed their `noUnusedParameters` warnings in angular/angular#15532
* When using Angular with Material and the users has `noUnusedParameters` enabled the library will be checked and will throw.

Fixes #4443
@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 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix P5 The team acknowledges the request but does not plan to address it, it remains open for discussion refactoring This issue is related to a refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants