| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Latest commit adds contextual this type in assignments of the form obj.xxx = function(...) and obj[xxx] = function(...). |
Sorry, something went wrong.
|
The one issue that remains for discussion here is whether the strongly typed this behavior should always be in effect or only be in effect in --noImplicitThis mode. It is technically a breaking change, but it also provides a better authoring experience (statement completion for this) and catches legitimate bugs. |
Sorry, something went wrong.
There was a problem hiding this comment.
I like this change.
Sorry, something went wrong.
| } | ||
|
|
||
| function getThisTypeFromContextualType(type: Type): Type { | ||
| return applyToContextualType(type, t => { |
There was a problem hiding this comment.
shouldn't applyToContextualType just be named mapType and replace mapType? It doesn't do anything specific to contextual types that I could see, and it's better than the current mapType because it skips return values of undefined. #Resolved
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, that makes sense.
Sorry, something went wrong.
| tests/cases/conformance/fixSignatureCaching.ts(640,13): error TS2304: Cannot find name 'window'. | ||
| tests/cases/conformance/fixSignatureCaching.ts(641,13): error TS2304: Cannot find name 'window'. | ||
| tests/cases/conformance/fixSignatureCaching.ts(704,18): error TS2339: Property 'prepareDetectionCache' does not exist on type '{}'. | ||
| tests/cases/conformance/fixSignatureCaching.ts(704,45): error TS2339: Property '_cache' does not exist on type '{ constructor: (userAgent: any, maxPhoneWidth: any) => void; mobile: () => any; phone: () => any; tablet: () => any; userAgent: () => any; userAgents: () => any; os: () => any; version: (key: any) => any; versionStr: (key: any) => any; is: (key: any) => any; match: (pattern: any) => any; isPhoneSized: (maxPhoneWidth: any) => any; mobileGrade: () => any; }'. |
There was a problem hiding this comment.
The purpose of this test isn't obvious without following the pointer back to issue #10697. Can you add one more comment at the top of the file like "this test originally failed by using 100% CPU and should now take less than 1 second to run". I'd like a faster way to know that these additional failures are not a problem.
Sorry, something went wrong.
| this // this: containing object literal type | ||
| this.mine | ||
| this.willDestroy | ||
| //this.willDestroy |
There was a problem hiding this comment.
just delete this, I think. #Resolved
Sorry, something went wrong.
There was a problem hiding this comment.
Done.
Sorry, something went wrong.
|
Summary of RWC breaks [1]: two good breaks from contextual typing of function assignment. Two breaks that show that Object.defineProperty and friends need to include ThisType<T>. One break that shows that choosing the contextual type of the object literal instead of the contextual type from a function call argument is incorrect. [1] Note that I haven't looked at all of RWC yet; it hit a stack overflow halfway through.
declare function f(elt: HTMLElement): void;
function outer() {
var self = this;
var args: Draggable;
args.helper = function () {
return f(this);
// error 'this: Draggable' is not assignable to 'HTMLElement'
// Property 'accessKey' is missing in 'Draggable'
};
}This catches code that is just wrong. I looked at Draggable and it's not at all related to HTMLElement. (This happened in two different projects.)
// Override the prototype method on C
CBase.prototype.debug = function() {
var self: C = this;
// error type 'CBase' is not assignable to 'C'
// Property '_subclassProperty' is missing in 'CBase'I'm not sure what to make of this. The comment makes it sound like the intention is to override the method on C, not on CBase. So probably this is a good error too.
Object.defineProperties(Array.prototype, {
empty: {
value: function() {
return this.length === 0;
}
}
})It looks like Object.defineProperties and related methods need to use ThisType to provide the correct type for this here.
function defineProperty(c: any, name: string, defaultValue: any) {
var backingFieldName = "_" + name;
Object.defineProperty(
c.prototype,
name,
{
get: function(): any {
if (typeof this[backingFieldName] === 'undefined') {
this[backingFieldName] = defaultValue;
}
return this[backingFieldName];
}, // code continues ...
});
};This would also be fixed by adding ThisType to Object.defineProperty because c.prototype: any and would avoid the noImplicitAny error.
interface MockJaxOptions {
url?: string;
responseTime?: number;
status?: number;
responseText?: string;
responseHeaders: StringMap<string>;
}
$.mockjax({
type: "GET",
url: "blah/blah/blah",
status: 200,
contentType: "application/json",
response: function() {
this.responseText = JSON.stringify({ stuff: "things" });
// error: property 'responseText' does not exist on '{ type: string ... }'
}
});This is bad because $.mockjax shouldn't really need to have its parameter be of type MockJaxOptions & ThisType<MockJaxOptions>. |
Sorry, something went wrong.
|
More breaks:
public getEmitOutput(resource: URI, absoluteWorkersResourcePath: string): WinJS.TPromise<Modes.IEmitOutput> { // TODO@Ben technical debt: worker cannot resolve paths absolute
let model = this.resourceService.get(resource);
let cssLinks: string[] = this.cssLinks || [];
// Custom Renderer to fix href in images
let renderer = new Marked.marked.Renderer();
let $this = this;
renderer.image = function(href: string, title: string, text: string): string {
let out = '<img src="' + $this.fixHref(resource, href) + '" alt="' + text + '"';
if (title) {
out += ' title="' + title + '"';
}
out += (this.options && this.options.xhtml) ? '/>' : '>';
~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'MarkedRenderer'.
~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'MarkedRenderer'.This seems like a good break. I can't find a property named 'options' in MarkedRenderer or in the class that contains getEmitOutput.
ko.observable.fn['toInt'] = function() {
return parseInt(this());
~~~~~~
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'KnockoutObservableFunctions<any>' has no compatible call signatures.
}This seems like an easy fix to knockout's d.ts. It's just that nobody ever noticed that KnockoutBindingHandler needed to have a line (): any at the beginning.
private setupEvents(xhr: XMLHttpRequest, request: AjaxRequest) {
const progressSubscriber = request.progressSubscriber;
xhr.ontimeout = function xhrTimeout(e) {
const {subscriber, progressSubscriber, request } = (<any>xhrTimeout);
if (progressSubscriber) {
progressSubscriber.error(e);
}
subscriber.error(new AjaxTimeoutError(this, request)); //TODO: Make betterer.
~~~~
!!! error TS2345: Argument of type 'XMLHttpRequestEventTarget' is not assignable to parameter of type 'XMLHttpRequest'.
!!! error TS2345: Property 'onreadystatechange' is missing in type 'XMLHttpRequestEventTarget'.
};This seems like a good break too. The comment even indicates that the author is suspicious that something is wrong.
export function main() {
describe('es5 decorators', () => {
it('should declare directive class', () => {
var MyDirective = Directive({}).Class({constructor: function() { this.works = true; }});
~~~~~
!!! error TS2339: Property 'works' does not exist on type '{ constructor: () => void; }'.
expect(new MyDirective().works).toEqual(true);
});This is probably another case where the type of the object literal needs to be intersected with the contextual type from the call signature. But it's in a test. It could just be a completely dynamic use of an undeclared property.
// tree benchmark in React
import {getIntParameter, bindAction} from 'angular2/src/test_lib/benchmark_util';
import * as React from './react.min';
var TreeComponent = React.createClass({
displayName: 'TreeComponent',
render: function() {
var treeNode = this.props.treeNode;
~~~~~
!!! error TS2339: Property 'props' does not exist on type '{ displayName: string; render: () => any; }'.This is evidence that React.createClass will need to update its d.ts too. |
Sorry, something went wrong.
|
I just tried this out and thought this would work: const person = {
firstName: 'First',
lastName: 'Last',
getName() {
// works!
return this.firstName + ' ' + this.lastName;
},
get name() {
// 'this' implicitly has type 'any' because it does not have a type annotation.
return this.firstName + ' ' + this.lastName;
},
};
But it looks like the getter isn't aware of the type of this. Is this behavior correct? |
Sorry, something went wrong.
|
Braden Snell (@bradenhs) Nope, that is not correct. But the code right now specifically works with the contextual type of this in functions and methods. It needs code to support getters and setters too. I'll update my review comment above so Anders Hejlsberg (@ahejlsberg) will see it when he gets back from vacaation. |
Sorry, something went wrong.
|
With latest commits we now properly type this in getters and setters. |
Sorry, something went wrong.
| @@ -22,9 +21,6 @@ tests/cases/conformance/types/thisType/thisTypeInAccessorsNegative.ts(16,22): er | |||
| } | |||
| const contextual: Foo = { | |||
There was a problem hiding this comment.
Can you move this test case to thisTypeInAccessors.ts so that we can see the types?
Sorry, something went wrong.
|
Latest commit puts all of the new functionality under the --noImplicitThis switch. This ensures there are no breaking changes as a result of this PR since the situations in which we strongly type this in object literal methods were previously errors under --noImplicitThis. |
Sorry, something went wrong.
|
In the SymbolDisplayBuilder, should we change the following? else if (type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType) {
if (inObjectTypeLiteral) {
writer.reportInaccessibleThisError();
}
writer.writeKeyword("this");
} |
Sorry, something went wrong.
|
Arthur Ozga (@aozgaa) No, that is an unrelated check (it reports an error if this is accessed in a type position within an object type literal). |
Sorry, something went wrong.
|
Mohamed Hegazy (@mhegazy) I think this one is good to do. Want to take a look? |
Sorry, something went wrong.
|
In this scenario, is it possible to infer this from the containing object literal? Or declare somehow that wrap does not change the context? let foo = {
hello: "hello",
f() {
return this.hello; // works
},
get p() {
return this.hello; // works
},
g: wrap(function () {
return this.hello; // TS2683:'this' implicitly has type 'any' because it does not have a type annotation.
})
}
|
Sorry, something went wrong.
|
I haven’t tested it but it should be possible with generics. declare function wrap<T, F extends (this: T) => any>(f: F): F |
Sorry, something went wrong.
|
Unfortunately that doesn't seem to work without specifying wrap<{ hello: string }>(function() { ... }). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
With this PR we strongly type this in methods of object literals and provide a facility for controlling the type of this through contextual typing. The new behavior is only enabled in --noImplicitThis mode.
The type of the expression this in a method of an object literal is now determined as follows:
Some examples:
In a similar manner, when --noImplicitThis is enabled and a function expression is assigned to a target of the form obj.xxx or obj[xxx], the contextual type for this in the function expression is obj:
In cases where an API produces a this value by transforming its arguments, a new ThisType<T> marker interface can be used to contextually indicate the transformed type. Specifically, when the contextual type for an object literal is ThisType<T> or an intersection including ThisType<T>, the type of this within methods of the object literal is T.
In the example above, the methods object in the argument to makeObject has a contextual type that includes ThisType<D & M> and therefore the type of this in methods within the methods object is { x: number, y: number } & { moveBy(dx: number, dy: number): number }. Notice how the type of the methods property simultaneously is an inference target and a source for the this type in methods.
The ThisType<T> marker interface is simply an empty interface declared in lib.d.ts. Beyond being recognized in the contextual type of an object literal, the interface acts like any empty interface.
Patterns similar to the above are used in several frameworks, including for example Vue and Ember. Using ThisType<T> we can now more accurately describe those frameworks.
Supercedes #8382. We revoked that PR because it always made the type of this in object literal methods be the type of the object literal. We now make that the default behavior, but allow the default to be overridden using a ThisType<T> contextual type.