fix: make material work with noUnusedParameters #4946

devversion · 2017-06-03T09:56:34Z

Recently Angular core fixed most of their noUnusedParameters warnings in Unused Parameters in Ng Factory code angular#15532 (this one is still left - Unused $event parameters in AOT generated code angular#17131)
When using Angular with Material and the users has noUnusedParameters enabled the library will be checked and will throw right now.

Note: I don't plan to add the noUnusedParameters to any build tsconfig files yet, because it would make the watching & live-reloading extremely bad. The noUnusedParameters should / can be tested on the CI in a follow-up.

Fixes #4443

jelbourn · 2017-06-05T19:43:50Z

src/lib/autocomplete/autocomplete.spec.ts

@@ -67,7 +67,7 @@ describe('MdAutocomplete', () => {
        }},
        {provide: Dir, useFactory: () => ({value: dir})},
        {provide: ScrollDispatcher, useFactory: () => {
-          return {scrolled: (delay: number, callback: () => any) => {
+          return {scrolled: (_delay: number, callback: () => any) => {


Why does this need an underscore? I'd prefer not to use _ for function arguments; is TS not smart enough to know that you've included an argument so that you can have access to a subsequent one?

Yeah unfortunately it's smart enough do detect this

jelbourn · 2017-06-05T22:44:06Z

src/lib/core/overlay/scroll/scroll-dispatcher.ts

@@ -105,7 +105,7 @@ export class ScrollDispatcher {
  getScrollContainers(elementRef: ElementRef): Scrollable[] {
    const scrollingContainers: Scrollable[] = [];

-    this.scrollableReferences.forEach((subscription: Subscription, scrollable: Scrollable) => {
+    this.scrollableReferences.forEach((_subscription: Subscription, scrollable: Scrollable) => {


Change this to this.scrollableReferences.keys().forEach(...) ?

Unfortunately there is no forEach() method for iterators. And if we want to use iterators here we need to enable down-leveling of those (read here and this requires a runtime shim for non-arrays.

jelbourn · 2017-06-05T22:48:05Z

src/lib/checkbox/checkbox.spec.ts

-  onCheckboxClick(event: Event) {}
-  onCheckboxChange(event: MdCheckboxChange) {}
+  onCheckboxClick(_event: Event) {}
+  onCheckboxChange(_event: MdCheckboxChange) {}


Should change these to be properties instead of methods since they're reassigned anyway, e.g.

onCheckboxClick: (Event?) => void = () => {};

(here and elsewhere that follows this pattern)

jelbourn · 2017-06-05T22:51:22Z

src/lib/core/datetime/native-date-adapter.ts

@@ -31,7 +31,7 @@ const DEFAULT_DAY_OF_WEEK_NAMES = {

 /** Creates an array and fills it with values. */
 function range<T>(length: number, valueFunction: (index: number) => T): T[] {
-  return Array.apply(null, Array(length)).map((v: undefined, i: number) => valueFunction(i));
+  return Array.apply(null, Array(length)).map((_v: undefined, i: number) => valueFunction(i));


We could eliminate this case by switching to a for loop... (which is faster anyway)

jelbourn · 2017-06-05T22:52:34Z

src/lib/core/style/focus-origin-monitor.ts

@@ -129,7 +129,7 @@ export class FocusOriginMonitor {
   * @param renderer The renderer to use to invoke the focus method on the element.
   * @param origin The focus origin.
   */
-  focusVia(element: HTMLElement, renderer: Renderer2, origin: FocusOrigin): void {
+  focusVia(element: HTMLElement, origin: FocusOrigin): void {


This one is a breaking change that will need Google to be updated, so we should do it in a separate PR

(I checked, there are people using it)

Agreed, prefixing with an underscore should work for now.

jelbourn · 2017-06-05T22:54:57Z

src/lib/radio/radio.spec.ts

@@ -143,7 +143,7 @@ describe('MdRadio', () => {
      expect(radioInstances[0].checked).toBe(false);

      let spies = radioInstances
-        .map((value, index) => jasmine.createSpy(`onChangeSpy ${index}`));
+        .map((_value, index) => jasmine.createSpy(`onChangeSpy ${index}`));


Could add the value into the spy name so that it's used.

devversion · 2017-06-06T12:55:57Z

@jelbourn Addressed your feedback. Expect the one with the Iterator.

jelbourn

LGTM aside from one comment

Can add merge-ready when done

jelbourn · 2017-06-06T15:51:52Z

src/lib/core/datetime/native-date-adapter.ts

@@ -31,7 +31,11 @@ const DEFAULT_DAY_OF_WEEK_NAMES = {

 /** Creates an array and fills it with values. */
 function range<T>(length: number, valueFunction: (index: number) => T): T[] {
-  return Array.apply(null, Array(length)).map((v: undefined, i: number) => valueFunction(i));
+  const valuesArray = [];


const valuesArray = Array(length);
Better to initialize it to the right length

* 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

angular-automatic-lock-bot · 2019-09-06T13:03:17Z

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.}

devversion added the pr: needs review label Jun 3, 2017

devversion assigned jelbourn Jun 3, 2017

devversion requested a review from jelbourn June 3, 2017 09:56

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 3, 2017

jelbourn reviewed Jun 5, 2017

View reviewed changes

jelbourn approved these changes Jun 6, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Jun 6, 2017

devversion force-pushed the fix/material-with-unused-locals branch 2 times, most recently from 4aee7f8 to 8774c91 Compare June 8, 2017 15:20

fix: make material work with noUnusedParameters

f177345

* 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 force-pushed the fix/material-with-unused-locals branch from 8774c91 to f177345 Compare June 8, 2017 16:19

devversion added the action: merge The PR is ready for merge by the caretaker label Jun 8, 2017

andrewseguin merged commit 4b98f21 into angular:master Jun 8, 2017

devversion deleted the fix/material-with-unused-locals branch June 8, 2017 21:33

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make material work with noUnusedParameters #4946

fix: make material work with noUnusedParameters #4946

devversion commented Jun 3, 2017

jelbourn Jun 5, 2017

devversion Jun 5, 2017

jelbourn Jun 5, 2017

devversion Jun 6, 2017

jelbourn Jun 5, 2017

jelbourn Jun 5, 2017

jelbourn Jun 5, 2017

devversion Jun 6, 2017

jelbourn Jun 5, 2017

devversion commented Jun 6, 2017

jelbourn left a comment

jelbourn Jun 6, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

fix: make material work with noUnusedParameters #4946

fix: make material work with noUnusedParameters #4946

Conversation

devversion commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion commented Jun 6, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 6, 2019