Skip to content

chore: enable strict apply, bind, and call #18172

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

Merged
merged 1 commit into from
Jan 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/bazel-tsconfig-build.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"noImplicitAny": true,
"noImplicitThis": true,
"importHelpers": true,
"strictBindCallApply": true,
"newLine": "lf",
"module": "es2015",
"moduleResolution": "node",
Expand Down
4 changes: 3 additions & 1 deletion src/cdk/a11y/key-manager/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ class FakeQueryList<T> extends QueryList<T> {
set length(_) { /* Empty setter for base class constructor */ }
get first() { return this.items[0]; }
toArray() { return this.items; }
some() { return this.items.some.apply(this.items, arguments); }
some(...args: [(value: T, index: number, array: T[]) => unknown, any?]) {
return this.items.some(...args);
}
notifyOnChanges() { this.changes.next(this); }
}

Expand Down
20 changes: 14 additions & 6 deletions src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1842,14 +1842,22 @@ describe('CdkDrag', () => {
body: document.body,
fullscreenElement: document.createElement('div'),
ELEMENT_NODE: Node.ELEMENT_NODE,
querySelectorAll: function() {
return document.querySelectorAll.apply(document, arguments);
querySelectorAll: function(...args: [string]) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be string[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

string[] is a string array of any length, [string] is an array of exactly one string

Copy link
Member

Choose a reason for hiding this comment

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

If we know it'll be one string, couldn't we just declare it like this?

querySelectorAll: (selector: string) {
  return document.querySelectorAll.call(document, selector);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that for these ones where we're just wrapping a function and passing things through, the author was trying to be defensive against future browser API changes by just grabbing all of the arguments and shoving them through. This style preserves that

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that I'm the author and I think I did it because it was shorter 😄

Copy link
Member

@devversion devversion Jan 14, 2020

Choose a reason for hiding this comment

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

I can only speak for the ZoneJS interceptor, where I also did it that way. I wanted to be on the safe side if a different ZoneJS version is used / when the signature changes. The typings are manually copied, so not very reliable IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah querySelectorAll probably isn't going to change much, but some do, addEventListener has definitely changed over the years

return document.querySelectorAll(...args);
},
addEventListener: function() {
document.addEventListener.apply(document, arguments);
addEventListener: function(...args: [
string,
EventListenerOrEventListenerObject,
(boolean | AddEventListenerOptions | undefined)?
]) {
document.addEventListener(...args);
},
removeEventListener: function() {
document.addEventListener.apply(document, arguments);
removeEventListener: function(...args: [
string,
EventListenerOrEventListenerObject,
(boolean | AddEventListenerOptions | undefined)?
]) {
document.addEventListener(...args);
}
};
const fixture = createComponent(DraggableInDropZone, [{
Expand Down
20 changes: 10 additions & 10 deletions src/cdk/overlay/fullscreen-overlay-container.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,28 @@ describe('FullscreenOverlayContainer', () => {
body: document.body,
fullscreenElement: document.createElement('div'),
fullscreenEnabled: true,
addEventListener: function(eventName: string, listener: Function) {
addEventListener: function(eventName: string, listener: EventListener) {
if (eventName === 'fullscreenchange') {
fullscreenListeners.add(listener);
} else {
document.addEventListener.apply(document, arguments);
document.addEventListener(eventName, listener);
}
},
removeEventListener: function(eventName: string, listener: Function) {
removeEventListener: function(eventName: string, listener: EventListener) {
if (eventName === 'fullscreenchange') {
fullscreenListeners.delete(listener);
} else {
document.addEventListener.apply(document, arguments);
document.addEventListener(eventName, listener);
}
},
querySelectorAll: function() {
return document.querySelectorAll.apply(document, arguments);
querySelectorAll: function(...args: [string]) {
Copy link
Member

Choose a reason for hiding this comment

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

Also should be string[]? There are a few more changes like this.

return document.querySelectorAll(...args);
},
createElement: function() {
return document.createElement.apply(document, arguments);
createElement: function(...args: [string, (ElementCreationOptions | undefined)?]) {
return document.createElement(...args);
},
getElementsByClassName: function() {
return document.getElementsByClassName.apply(document, arguments);
getElementsByClassName: function(...args: [string]) {
return document.getElementsByClassName(...args);
}
};

Expand Down
37 changes: 18 additions & 19 deletions src/cdk/portal/portal.spec.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
import {inject, ComponentFixture, TestBed} from '@angular/core/testing';
import {CommonModule} from '@angular/common';
import {
NgModule,
ApplicationRef,
Component,
ViewChild,
ViewChildren,
QueryList,
ViewContainerRef,
ComponentFactoryResolver,
Optional,
Injector,
ApplicationRef,
TemplateRef,
ComponentRef,
ElementRef,
Injector,
NgModule,
Optional,
QueryList,
TemplateRef,
Type,
ViewChild,
ViewChildren,
ViewContainerRef,
} from '@angular/core';
import {CommonModule} from '@angular/common';
import {CdkPortal, CdkPortalOutlet, PortalModule} from './portal-directives';
import {Portal, ComponentPortal, TemplatePortal, DomPortal} from './portal';
import {ComponentFixture, inject, TestBed} from '@angular/core/testing';
import {DomPortalOutlet} from './dom-portal-outlet';
import {ComponentPortal, DomPortal, Portal, TemplatePortal} from './portal';
import {CdkPortal, CdkPortalOutlet, PortalModule} from './portal-directives';


describe('Portals', () => {
Expand Down Expand Up @@ -377,10 +378,9 @@ describe('Portals', () => {
it('should use the `ComponentFactoryResolver` from the portal, if available', () => {
const spy = jasmine.createSpy('resolveComponentFactorySpy');
const portal = new ComponentPortal(PizzaMsg, undefined, undefined, {
resolveComponentFactory: (...args: any[]) => {
resolveComponentFactory: <T>(...args: [Type<T>]) => {
spy();
return componentFactoryResolver.resolveComponentFactory
.apply(componentFactoryResolver, args);
return componentFactoryResolver.resolveComponentFactory(...args);
}
});

Expand Down Expand Up @@ -560,10 +560,9 @@ describe('Portals', () => {
it('should use the `ComponentFactoryResolver` from the portal, if available', () => {
const spy = jasmine.createSpy('resolveComponentFactorySpy');
const portal = new ComponentPortal(PizzaMsg, undefined, undefined, {
resolveComponentFactory: (...args: any[]) => {
resolveComponentFactory: <T>(...args: [Type<T>]) => {
spy();
return componentFactoryResolver.resolveComponentFactory
.apply(componentFactoryResolver, args);
return componentFactoryResolver.resolveComponentFactory(...args);
}
});

Expand Down
2 changes: 1 addition & 1 deletion src/cdk/testing/private/e2e/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {getElement, FinderResult} from './query';
*/
export async function pressKeys(...keys: string[]) {
const actions = browser.actions();
await actions.sendKeys.call(actions, keys).perform();
await actions.sendKeys(...keys).perform();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/cdk/testing/testbed/fake-events/event-objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {ModifierKeys} from '@angular/cdk/testing';
*/
export function createMouseEvent(type: string, x = 0, y = 0, button = 0) {
const event = document.createEvent('MouseEvent');
const originalPreventDefault = event.preventDefault;
const originalPreventDefault = event.preventDefault.bind(event);

event.initMouseEvent(type,
true, /* canBubble */
Expand All @@ -39,7 +39,7 @@ export function createMouseEvent(type: string, x = 0, y = 0, button = 0) {
// IE won't set `defaultPrevented` on synthetic events so we need to do it manually.
event.preventDefault = function() {
Object.defineProperty(event, 'defaultPrevented', { get: () => true });
return originalPreventDefault.apply(this, arguments);
return originalPreventDefault();
};

return event;
Expand Down
8 changes: 4 additions & 4 deletions src/cdk/testing/testbed/task-state-zone-interceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,17 @@ export class TaskStateZoneInterceptor {
// the proxy zone keeps track of the previous task state, so we can just pass
// this as initial state to the task zone interceptor.
const interceptor = new TaskStateZoneInterceptor(zoneSpec.lastTaskState);
const zoneSpecOnHasTask = zoneSpec.onHasTask;
const zoneSpecOnHasTask = zoneSpec.onHasTask.bind(zoneSpec);

// We setup the task state interceptor in the `ProxyZone`. Note that we cannot register
// the interceptor as a new proxy zone delegate because it would mean that other zone
// delegates (e.g. `FakeAsyncTestZone` or `AsyncTestZone`) can accidentally overwrite/disable
// our interceptor. Since we just intend to monitor the task state of the proxy zone, it is
// sufficient to just patch the proxy zone. This also avoids that we interfere with the task
// queue scheduling logic.
zoneSpec.onHasTask = function() {
zoneSpecOnHasTask.apply(zoneSpec, arguments);
interceptor.onHasTask.apply(interceptor, arguments);
zoneSpec.onHasTask = function(...args: [ZoneDelegate, Zone, Zone, HasTaskState]) {
zoneSpecOnHasTask(...args);
interceptor.onHasTask(...args);
};

return zoneSpec[stateObservableSymbol] = interceptor.state;
Expand Down
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"noImplicitAny": true,
"noImplicitThis": true,
"skipLibCheck": true,
"strictBindCallApply": true,
"target": "es2015",
"lib": ["es5", "es2015", "dom"],
"types": ["jasmine"],
Expand Down