Typescript 2.6 #367

bryanforbes · 2017-11-09T20:31:29Z

Type: enhancement

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Relates to dojo/meta#210

agubler

A few questions

agubler · 2017-11-27T08:27:09Z

src/QueuingEvented.ts


-	emit<E extends EventObject>(event: E): void {
-		super.emit(event);
+(function (proto) {


How come this is needed?

Yes, couldn't something like this be accomplished with the mixin pattern?

I'll look into the mixin pattern

I think this is left-over from when the signature of emit and on were much more complex than they are now and I was trying to avoid repeating the signatures in QueingEvented. I've fixed these up locally to be methods on QueingEvented.

agubler · 2017-11-27T08:27:55Z

src/Evented.ts

-			const handles = Object.keys(listenerMapArg).map((type) => this.on(type, listenerMapArg[type]));
+	on<K extends keyof M>(type: K, listener: EventedCallbackOrArray<K, M[K]>): Handle;
+	on(type: T, listener: EventedCallbackOrArray<T, O>): Handle;
+	on(type: any, listener: EventedCallbackOrArray<any, any>): Handle {


Assume this can't be T | K?

Correct. K and T need to be specifically associated with the correct EventedCallbackOrArray. Using a union type means that T could be associated with EventedCallbackOrArray<K, M[K]> which we don't want.

agubler · 2017-11-27T08:28:17Z

src/Evented.ts

+	// The following member is purely so TypeScript remembers the type of `M` when extending so
+	// that the utilities in `on.ts` will work
+	// tslint:disable-next-line
+	protected __typeMap__?: M;


This seems is a bit hacky :(

Yeah, feels dirty to me too... 😢

Yeah, it's dirty, but I'm not sure if there's any other way. TypeScript isn't able to remember the type map for use with the stand-alone on() function without it.

I've filed an issue with a reduced test case outlining this problem. It feels like a bug.

agubler · 2017-11-27T08:28:30Z

src/Evented.ts

 		this.listenersMap.forEach((method, type) => {
-			if (isGlobMatch(type, event.type)) {
+			if (isGlobMatch(<any> type, event.type)) {


Why do we need <any> here?

Yes, lets try to comment whenever we have to cast as any (as well as prefer the type as any instead of the arrow brackets.

Because type is a generic that can be anything and isGlobMatch() requires string | symbol. Also, casting type to string | symbol doesn't work either.

let's try to comment whenever we have to cast as any

As in, add a comment to code for future generations, please.

agubler · 2017-11-27T08:28:53Z

src/QueuingEvented.ts

 		this.listenersMap.forEach((method, type) => {
 			if (isGlobMatch(type, event.type)) {
 				hasMatch = true;
 			}
 		});

 		if (!hasMatch) {
+			// @ts-ignore
 			let queue = this._queue.get(event.type);


What are we ignoring here?

yeah, we really should comment why we are deciding to ignore something and would much prefer casting (and a comment)... I have yet to see a valid use case yet for // @ts-ignore in our code base.

These were because _queue and listenersMap weren't accessible outside of the class declaration of QueuingEvented. Since I've moved the functions into the class definition, these comments are no longer needed.

agubler · 2017-11-27T08:29:58Z

src/async/timing.ts

 		if (Date.now() - milliseconds > start) {
 			return Promise.reject<T>(reason);
 		}
+		if (typeof value === 'function') {


Is this new functionality?

Doesn't look like it's covered in tests

It's not new functionality (value was T | (() => T | Thenable<T>) before my changes), but it wasn't handled properly before. You're right that we need to test it.

Tests have been added

agubler · 2017-11-27T08:33:03Z

Would be great if @kitsonk could also review

agubler · 2017-11-27T08:34:49Z

src/Destroyable.ts

-		const { handles } = this;
-		handles.push(handle);
+	own(handles: Handle | Handle[]): Handle {
+		const handle = Array.isArray(handles) ? createCompositeHandle(...handles) : handles;


We are missing a test branch here

Tests added

kitsonk · 2017-11-27T10:22:51Z

src/Destroyable.ts

-		handles.push(handle);
+	own(handles: Handle | Handle[]): Handle {
+		const handle = Array.isArray(handles) ? createCompositeHandle(...handles) : handles;
+		const { handles: _handles } = this;


Why the rename?

Locally, handles is Handle | Handle[] and this.handles is Handle[]. Assigning works fine initially, but when it gets into the destroy() function returned, the compiler reverts to thinking that handles is Handle | Handle[] and requires a cast. I thought a rename was cleaner than a cast.

kitsonk · 2017-11-27T15:48:16Z

src/Evented.ts

+	// The following member is purely so TypeScript remembers the type of `M` when extending so
+	// that the utilities in `on.ts` will work
+	// tslint:disable-next-line
+	protected __typeMap__?: M;


Yeah, feels dirty to me too... 😢

kitsonk · 2017-11-27T15:49:44Z

src/Evented.ts

 		this.listenersMap.forEach((method, type) => {
-			if (isGlobMatch(type, event.type)) {
+			if (isGlobMatch(<any> type, event.type)) {


Yes, lets try to comment whenever we have to cast as any (as well as prefer the type as any instead of the arrow brackets.

kitsonk · 2017-11-27T15:51:22Z

src/QueuingEvented.ts


-	emit<E extends EventObject>(event: E): void {
-		super.emit(event);
+(function (proto) {


Yes, couldn't something like this be accomplished with the mixin pattern?

kitsonk · 2017-11-27T15:52:31Z

tests/unit/aspect.ts

@@ -4,7 +4,7 @@ const { registerSuite } = intern.getInterface('object');
 const { assert } = intern.getPlugin('chai');
 import * as sinon from 'sinon';
 import * as aspect from '../../src/aspect';
-import { Handle } from '@dojo/interfaces/core';
+import { Handle } from '../../src/interfaces';


We added support for symbols, but no tests for symbols.

…ojo/interfaces

bryanforbes added the enhancement label Nov 9, 2017

bryanforbes requested a review from agubler November 9, 2017 20:31

bryanforbes force-pushed the typescript-2.6 branch 2 times, most recently from 630864f to 0aa23d6 Compare November 14, 2017 16:53

bryanforbes added 10 commits November 14, 2017 12:39

Update to Intern 4.1.0 and TypeScript 2.6.1 and turn on strict

4f6541b

Rework Evented interface and drop dependency on interfaces

0b34d4e

Adjust Evented and Destroyable APIs

f632ea7

Push tarballs for CI

b39c89c

Update for CI

28ec9be

Push to progress

14faaa1

Updates

b43a228

Updates

6414973

Use "capabilities+" so idle-timeout doesn't get overwritten

b6274c8

Updates

d3f7248

bryanforbes force-pushed the typescript-2.6 branch from 0aa23d6 to d3f7248 Compare November 14, 2017 20:00

bryanforbes added 6 commits November 14, 2017 17:54

Tweaks

8b176e4

Fixes to request interfaces and exports

9018b8a

Add back @dojo/interfaces for loader interfaces

5f08de9

Bump minor version and update dependencies

6929742

Update on() to work with Evented

94865e8

Add a protected property to make the map type extractable in functions

dfa735c

agubler reviewed Nov 27, 2017

View reviewed changes

agubler requested a review from kitsonk November 27, 2017 08:32

agubler reviewed Nov 27, 2017

View reviewed changes

kitsonk reviewed Nov 27, 2017

View reviewed changes

bryanforbes added 4 commits November 27, 2017 14:14

Updates based on feedback

76e76ef

Updates based on feedback

7b4dd60

Update interfaces tarball, add Safari, and add core@0.2.1 as devDep

94e59fb

Use global-style during development to force intern to use its own @d…

6c76d34

…ojo/interfaces

Address feedback

498d9fb

bryanforbes force-pushed the typescript-2.6 branch from f571bcf to 498d9fb Compare November 29, 2017 22:36

Remove interfaces tarball and use 0.2.0

5fccf38

bryanforbes merged commit f826f65 into dojo:master Nov 30, 2017

bryanforbes deleted the typescript-2.6 branch November 30, 2017 16:20

mgroenhoff mentioned this pull request Dec 14, 2017

tslib must be a dependency and not a devDependency #372

Closed

dylans added this to the beta.5 milestone Jan 4, 2018

mgroenhoff mentioned this pull request Jan 11, 2018

Missing 'tslib' when TypeScript should use external helpers dojo/meta#226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript 2.6 #367

Typescript 2.6 #367

bryanforbes commented Nov 9, 2017

agubler left a comment

agubler Nov 27, 2017

kitsonk Nov 27, 2017

bryanforbes Nov 27, 2017

bryanforbes Nov 27, 2017

agubler Nov 27, 2017

bryanforbes Nov 27, 2017

agubler Nov 27, 2017

kitsonk Nov 27, 2017

bryanforbes Nov 29, 2017

bryanforbes Nov 29, 2017

agubler Nov 27, 2017

kitsonk Nov 27, 2017

bryanforbes Nov 27, 2017 •

edited

kitsonk Nov 28, 2017

agubler Nov 27, 2017

kitsonk Nov 27, 2017

bryanforbes Nov 27, 2017

agubler Nov 27, 2017

agubler Nov 27, 2017

bryanforbes Nov 27, 2017 •

edited

bryanforbes Nov 27, 2017

agubler commented Nov 27, 2017

agubler Nov 27, 2017

bryanforbes Nov 27, 2017

kitsonk Nov 27, 2017

bryanforbes Nov 27, 2017

kitsonk Nov 28, 2017

kitsonk Nov 27, 2017

kitsonk Nov 27, 2017

kitsonk Nov 27, 2017

kitsonk Nov 27, 2017

Typescript 2.6 #367

Typescript 2.6 #367

Conversation

bryanforbes commented Nov 9, 2017

agubler left a 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

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

bryanforbes Nov 27, 2017 • edited

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

bryanforbes Nov 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler commented Nov 27, 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

bryanforbes Nov 27, 2017 •

edited

bryanforbes Nov 27, 2017 •

edited