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

EventedConstructor incorrectly typed #113

Open
devpaul opened this issue Feb 9, 2017 · 4 comments
Open

EventedConstructor incorrectly typed #113

devpaul opened this issue Feb 9, 2017 · 4 comments

Comments

@devpaul
Copy link
Member

devpaul commented Feb 9, 2017

Evented isn't created with declare. It's a traditional es5 class function.

define(["./aspect", "./on"], function(aspect, on){
	// module:
	//		dojo/Evented

 	"use strict";
 	var after = aspect.after;
	function Evented(){
		// summary:
		//		A class that can be used as a mixin or base class,
		//		to add on() and emit() methods to a class
		//		for listening for events and emitting events:
		// example:
		//		|	define(["dojo/Evented", "dojo/_base/declare", "dojo/Stateful"
		//		|	], function(Evented, declare, Stateful){
		//		|		var EventedStateful = declare([Evented, Stateful], {...});
		//		|		var instance = new EventedStateful();
		//		|		instance.on("open", function(event){
		//		|		... do something with event
		//		|	 });
		//		|
		//		|	instance.emit("open", {name:"some event", ...});
	}
	Evented.prototype = {
		on: function(type, listener){
			return on.parse(this, type, listener, function(target, type){
				return after(target, 'on' + type, listener, true);
			});
		},
		emit: function(type, event){
			var args = [this];
			args.push.apply(args, arguments);
			return on.emit.apply(on, args);
		}
	};
	return Evented;
});

This needs fixing:

interface EventedConstructor extends _base.DeclareConstructor<Evented> {
		new (params?: Object): Evented;
	}
@dylans
Copy link
Member

dylans commented Feb 9, 2017

@mmckenziedev have you run into an issue with this yet?

@mmckenziedev
Copy link
Contributor

mmckenziedev commented Feb 9, 2017

@dylans I haven't had problems using Declare to mix this in to other classes.

I agree that technically this class is an ES5 class and shouldn't use DeclareConstructor. However, in practice, if you're using Declare to create a new class and you try to mix this in, if it is not a DeclareConstructor then it won't work properly due to the way Declare is defined.
_base.d.ts#L620

If it's not breaking anything I would prefer to leave this as it is for now. When Typescript 2.2 comes out, and we can finally have a better expression syntax for mixins, then we could refactor the way Declare works.

https://github.com/Microsoft/TypeScript/wiki/Roadmap#22-february-2017
microsoft/TypeScript#13743

@devpaul
Copy link
Member Author

devpaul commented Feb 9, 2017

Yeah, the pattern used with DeclareConstructor makes this a sticky issue to resolve.

It's usage should be OK with declare, but I was trying to extend Evented like a class

export default class<T> extends Evented implements StateMachineEvents<T> {

Which results in the error

error TS2510: Base constructors must all have the same return type.

The error may be due to Evented having two different constructors:

interface EventedConstructor extends _base.DeclareConstructor<Evented> {
	new (params?: Object): Evented;
}

interface DeclareConstructor<T> {
	new (...args: any[]): T & DeclareCreatedObject;

I was able to resolve by "fixing" the typings

export interface FixedEventedConstructor {
	new (params?: Object): dojo.Evented;
}
export const FixedEvented: FixedEventedConstructor = <any> Evented;

I haven't had a chance to play with TypeScript 2.2. Hopefully we'll be able to define mixin classes as purely ambient declarations that return functional constructors. If we have to apply it as a pattern in code and actually have to return a class that extends from a declare statement, I would be concerned about losing the metadata added by declare to allow for functional chaining via inherited. Hopefully it'll be the former and we'll have a good way of declaring define.

Anyway, all of this created a rabbit hole I knew I didn't want to go down on my own.

@schontz
Copy link

schontz commented Mar 20, 2019

This issue bit me as well. Is #113 (comment) the prescribed way around it for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants