Add TypeScript definition #17

dinoboff · 2017-12-04T01:53:36Z

It including declarations in the npm package using default declaration locations.

Note that by default it doesn’t allow to map an event data type to each event; in TS, Emittery can be aliased to interface to support it. As of TS 2.6, that optional interface doesn’t support symbol.

This PR includes some rather slow tests. To skip those tests, ava can exclude test ending with ‘[slow]’, i.e. ava —match ‘!*[slow]’.

Fix #13

novemberborn

The TS config and dependencies should be moved into the root I think, if only so editors can verify the index.d.ts file, which didn't seem to work for me in Atom until I moved things up.

I've iterated a bit on your type definition, using keyof to allow typing of the event name and data. Please have a look at 9a73e32 — curious to know what you think of that approach.

The one use case I couldn't solve elegantly is defining events that do not have any data. Either we make all data optional, or it requires a separate interface which is a bit iffy. But best I can tell you can't do conditions inside a generic.

novemberborn · 2017-12-04T14:19:51Z

index.d.ts

+// Project: emittery
+// Definitions by: Sindre Sorhus <sindresorhus.com>
+
+export = Emittery;


Should this use export default Emittery instead?

@novemberborn if defined as an es6 export, VSC's Intellisense in js files expects a default export and only provide autocomplete and description on that default value.

ps: i.e. VSC expects const Emittery = require('emittery').default;.

Ah that makes sense I suppose. Best stick to export = Emittery then, since this module assumes CJS not ESM.

novemberborn · 2017-12-04T14:22:38Z

index.d.ts

+		 *
+		 * @returns Unsubscribe method.
+		 */
+		on<U>(eventName: T, listener: (eventData: U) => any): () => void;


I'm not sure how useful this is. TS will infer U based on the listener anyhow.

It might be interesting to type the return value of the listener as void | Promise<void>. This would signal that the listener's return value is ignored, unless it's a promise. Though it might enforce use of async/await since it's too easy otherwise to return a promise chain that resolves with a non-void value.

TS will complain if the listener returns / resolves something. Is it worth it?

I know, and it's why I'm undecided. I'd like to be more explicit about what happens when a promise is returned, but as you said typing the return value may just get in the way.

novemberborn · 2017-12-04T15:23:28Z

The one use case I couldn't solve elegantly is defining events that do not have any data. Either we make all data optional, or it requires a separate interface which is a bit iffy. But best I can tell you can't do conditions inside a generic.

Though microsoft/TypeScript#12424 might solve this.

keyof also precludes us from typing Symbol event names.

novemberborn · 2017-12-04T15:34:12Z

examples/typed.ts

@@ -0,0 +1,41 @@
+import Emitter from '../';


This should import as Emittery.

dinoboff · 2017-12-04T17:23:29Z

@novemberborn 👍

I setup typescript in the examples/ folder to not make Typescript a dependency but I don't personally mind.

ps: Is it ok to rebase it here?

dinoboff · 2017-12-05T11:50:29Z

@novemberborn

Is there anyway to get something like that to work:

type PickVoid<T> = {
	[P in keyof T]: T[P] & void;
}


class Emittery<Mapped, EventOnly=PickVoid<Mapped>> {
    
    emit<Name extends keyof EventOnly>(eventName: Name, eventData?: undefined): Promise<void>;
    emit<Name extends keyof Mapped>(eventName: Name, eventData: Mapped[Name]): Promise<void> {
        console.log(eventName, eventData);
        return Promise.resolve()
    }
}

const ee = new Emittery<{value: string, close: void}>()

ee.emit('start');        // report error as expected
ee.emit('value');        // Allows it :(
ee.emit('close', 'foo'); // report error as expected

playground

novemberborn · 2017-12-05T14:11:23Z

ps: Is it ok to rebase it here?

Yea do whatever you need to do. Though we'll probably squash and merge so rebasing is not a necessity.

Is there anyway to get something like that to work:

type PickVoid<T> = {
	[P in keyof T]: T[P] & void;
}


class Emittery<Mapped, EventOnly=PickVoid<Mapped>> {
    
    emit<Name extends keyof EventOnly>(eventName: Name, eventData?: undefined): Promise<void>;
    emit<Name extends keyof Mapped>(eventName: Name, eventData: Mapped[Name]): Promise<void> {
        console.log(eventName, eventData);
        return Promise.resolve()
    }
}

const ee = new Emittery<{value: string, close: void}>()

ee.emit('start');        // report error as expected
ee.emit('value');        // Allows it :(
ee.emit('close', 'foo'); // report error as expected

This doesn't work because EventOnly has the same keys as Mapped. T[P] & void doesn't matter here.

I think ideally we have Emittery, which allows all string event names, with any data:

class Emittery<Mapped = {[eventName: string]: any}, EventOnly = {[eventName: string]: any}> {}

Then we'll let you type which events you want to emit, and their data, while still allowing all string event names without data:

class Example extends Emittery<{foo: 'bar'}> {}

And finally we'd let you type which events have no data:

class Example extends Emittery<{foo: 'bar'}, {baz: true}> {}

Unfortunately we can't derive EventOnly from Mapped by filtering those keys that have void values. Still, this seems like the best available solution.

Note that the PR currently uses EventOnly = Mapped which disallows unknown event names, but allows all known event names to be used without data. Perhaps that's OK after all?

@dinoboff we can't generalize Symbol support like this, though presumably you could add signatures for on(eventName: symbol, eventData: any) etc in the subclass. I think that's a decent argument for not supporting symbols at all. What do you think?

dinoboff · 2017-12-09T13:44:01Z

@novemberborn Not necessarily. It can support Symbol even if the type declaration doesn't reflect it; When authoring JS code VSC doesn't complain if I use a symbol as key. Is it an issue in Atom?

An alternative is to define a basic type declaration and provide an interface with a Mapped Event name/data. When authoring in TS could use either depending of the need for symbol.

Even when Symbol support for index lands in TS (TS 2.7 AFAIK), a simpler interface by default might be better for JS authoring.

novemberborn · 2017-12-11T16:28:27Z

An alternative is to define a basic type declaration and provide an interface with a Mapped Event name/data. When authoring in TS could use either depending of the need for symbol.

Even when Symbol support for index lands in TS (TS 2.7 AFAIK), a simpler interface by default might be better for JS authoring.

Yes I like that. Great to hear that TS is improving their symbol support, for now we should have a basic interface that could work for symbols and strings, and as TypeScript improves we can extend the mapped interface to also support symbols.

novemberborn

@dinoboff I've left a few more comments on the type definition, examples and the tests. I like your use of snapshots!

novemberborn · 2017-12-11T16:30:02Z

examples/clock.js

+
+	async tick() {
+		if (this._tick <= 0) {
+			this.emit('error', new Error('not started'));


Example nitpick, but should this be await this.emit()?

novemberborn · 2017-12-11T16:30:49Z

examples/clock.js

+	}
+
+	async tick() {
+		if (this._tick <= 0) {


Looks like this should check _startedAt, or perhaps _timer === null?

novemberborn · 2017-12-11T16:31:36Z

examples/clock.js

+	async tick() {
+		if (this._tick <= 0) {
+			this.emit('error', new Error('not started'));
+			this.stop();


Looks like this.stop() will throw in this code path.

novemberborn · 2017-12-11T16:31:50Z

examples/clock.js

+		this._timer = null;
+		this._startedAt = 0;
+
+		this.emit('started');


'stopped'?

novemberborn · 2017-12-11T16:34:43Z

examples/emit.js

+
+const Emittery = require('../');
+
+class MyEmitter extends Emittery {}


Would be nice to use this of an example where you don't need to extend Emittery.

novemberborn · 2017-12-11T16:38:58Z

index.d.ts

+	/**
+	 * A map of event names to the data type they emit.
+	 */
+	interface IEvents {


Personally I avoid prefixing interfaces with I. I suppose the TS community is split on this?

@sindresorhus what do you prefer?

I don't mind. Just following TSlint advice.

I prefer without the prefix too.

novemberborn · 2017-12-11T16:40:23Z

index.d.ts

+	 * Alternative interface for an Emittery object; must list supported events
+	 * the data type they emit.
+	 */
+	export interface IMapped<Events extends IEvents> {


I think we should still add the second generic, allowing you to specify which events have no data.

novemberborn · 2017-12-11T16:42:26Z

test/fixtures/compiles/off.ts

+import Emittery = require('../../..');
+
+const ee = new Emittery();
+const listener = () => undefined;


listener is unused.

novemberborn · 2017-12-11T16:45:52Z

test/types.js

+	const filesWithErrors = errors
+		.map(err => (err.file ? err.file.fileName : null))
+		.filter(Boolean)
+		.filter(unique);


You could pass the boolean-filtered array to new Set() to get unique values. If you do the same with fileNames you can still use t.deepEqual().

It including declarations in the npm package using default declaration locations. Note that by default it doesn’t allow to map an event data type to each event; in TS, Emittery can be aliased to interface to support it. As of TS 2.6, that optional interface doesn’t support symbol. This PR includes some rather slow tests. To skip those tests, ava should exclude test ending with ‘[slow]’, i.e. `ava —match ‘!*[slow]’` Fix sindresorhus#13

Rather than copying the definition file, re-export it from a committed `legacy.d.ts`. Rename `index.d.ts` so the definition can be imported.

--strict means future minor releases may end up working stricter in a non-backwards compatible manner.

* allowJs was superfluous with the explicit include option * The include option matches the default behavior (without allowJs) * The exclude option can be simplified

Create a `Typed` subclass of `Emittery` onto which we can hang the TypeScript method overloads.

I found "unsubscribe to an event" too ambiguous.

* Mirror the README * Remove unused JSDoc annotations (as far as VS Code and Atom are concerned) * Alias the unsubscribe function which leads to nicer type hints in VS Code and Atom

* Use glob to find files * Rely on CWD, which AVA already sets to the project root

novemberborn · 2018-01-02T18:18:48Z

@dinoboff I've done some iterations on your PR, please see the commits I've just pushed. I think I've got a nicer approach for mapping event types now.

I'm not sure what to do with the documentation — VS Code and Atom show much less detail than I'd hoped, especially for Emittery.Typed. We'd need to duplicate the README twice more to have docs for all methods in all use cases. Is it even worth having these docs in the TypeScript definition?

@sindresorhus this adds an empty subclass to Emittery itself. Please let me know what you think about that. It's the best way I've found of supporting generic Emittery usage while encouraging people to type their events. I also reworded off() in the docs.

sindresorhus · 2018-01-02T19:59:47Z

I'm not sure what to do with the documentation — VS Code and Atom show much less detail than I'd hoped, especially for Emittery.Typed.

What is it missing? Maybe we could open some issues about it on those projects or the TS issue tracker.

And can you quickly mention Emittery.Typed in the readme for TS users? So they know upfront what they should use. You can just refer to the type docs for more info.

this adds an empty subclass to Emittery itself. Please let me know what you think about that. It's the best way I've found of supporting generic Emittery usage while encouraging people to type their events. I also reworded off() in the docs.

Yeah, I'm ok with that.

novemberborn · 2018-01-03T13:28:35Z

I'm not sure what to do with the documentation — VS Code and Atom show much less detail than I'd hoped, especially for Emittery.Typed.

What is it missing? Maybe we could open some issues about it on those projects or the TS issue tracker.

The Emittery import doesn't show any documentation, though Emittery.Typed's class documentation does show up. But none of the method overloads of Emittery.Typed have documentation. I suppose that's somewhat sensible being an overload and all but if we'd want that to be visible we'd have to duplicate the docs for each overload.

And can you quickly mention Emittery.Typed in the readme for TS users? So they know upfront what they should use. You can just refer to the type docs for more info

Yes good catch.

I'm having second thoughts on how legacy.d.ts simply aliases Emittery.d.ts. It leads to this problem:

import E = require('.')
import L = require('./legacy')

let v: E
v = new L()
console.log(v instanceof E)

This passes type checks, but of course the classes are different so v instanceof E is actually false. Functionally both classes are the same though. Perhaps we can live with this edge case? @dinoboff what do you think?

dinoboff · 2018-01-03T16:05:08Z

@novemberborn I am not sure about the issue. I am guessing you would prefer TS to complain about typeof?

novemberborn · 2018-01-04T13:50:46Z

@novemberborn I am not sure about the issue. I am guessing you would prefer TS to complain about typeof?

@dinoboff yea. But turns out even if I copy Emittery.d.ts into legacy.d.ts my code snippet above behaves the same way, so I don't think this is actually an issue.

novemberborn · 2018-01-04T13:58:44Z

@dinoboff @sindresorhus I think this is ready now. I've left the method documentation in the definition file for now, let me know if you'd want me to remove this instead.

sindresorhus · 2018-01-04T14:20:17Z

@novemberborn Can you land it in two commits, one commit with @dinoboff's commits squashed and one with yours?

Closes #17. Alias Emittery definition for legacy module. Rather than copying the definition file, re-export it from a committed `legacy.d.ts`. Rename `index.d.ts` so the definition can be imported. Use individual strictness compiler options. --strict means future minor releases may end up working stricter in a non-backwards compatible manner. Clean up file inclusion: * allowJs was superfluous with the explicit include option * The include option matches the default behavior (without allowJs) * The exclude option can be simplified Add additional compiler options and reformat tsconfig.json. Update @types/node. Install ts-node and select it through npx in TypeScript example. Clean up clock examples. Improve usability of "mapped" interface. Create a `Typed` subclass of `Emittery` onto which we can hang the TypeScript method overloads. Reword off() and offAny() descriptions. I found "unsubscribe to an event" too ambiguous. Update TS code comments: * Mirror the README * Remove unused JSDoc annotations (as far as VS Code and Atom are concerned) * Alias the unsubscribe function which leads to nicer type hints in VS Code and Atom Clean up remaining examples. Remove unused snapshot. Clean up tests: * Use glob to find files * Rely on CWD, which AVA already sets to the project root Add TypeScript support to README.

novemberborn · 2018-01-04T14:30:48Z

Thanks for kickstarting this, as well as being so appreciative of the back and forth iterations @dinoboff!

sindresorhus · 2018-01-04T14:32:43Z

Thanks @dinoboff and @novemberborn. This is amazing :)

dinoboff · 2018-01-04T23:12:01Z

Likewise @novemberborn, thanks.

sindresorhus · 2018-01-17T18:15:43Z

@dinoboff Would you be interested in joining the project as a maintainer? @novemberborn and I would be happy to have you if you're interested :)

novemberborn reviewed Dec 4, 2017

View reviewed changes

examples/typed.ts Outdated

@@ -0,0 +1,41 @@

import Emitter from '../';

Copy link

Collaborator

novemberborn Dec 4, 2017

Choose a reason for hiding this comment

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

This should import as Emittery.

dinoboff force-pushed the chore/typescript branch from 7e8292a to 229c8d4 Compare December 4, 2017 17:20

dinoboff force-pushed the chore/typescript branch 3 times, most recently from 693563f to 92acf58 Compare December 11, 2017 00:44

dinoboff force-pushed the chore/typescript branch from 92acf58 to f09e945 Compare December 11, 2017 16:44

novemberborn reviewed Dec 11, 2017

View reviewed changes

dinoboff force-pushed the chore/typescript branch 5 times, most recently from f0cfc25 to f6972b4 Compare December 11, 2017 17:18

dinoboff force-pushed the chore/typescript branch from a7f1d96 to bb18108 Compare December 16, 2017 00:48

fixup! Add TypeScript definition

f39dbcd

dinoboff force-pushed the chore/typescript branch from bb18108 to 88c1fa4 Compare December 16, 2017 00:52

dinoboff and others added 7 commits December 16, 2017 00:52

fixup! Add TypeScript definition

88c1fa4

Alias Emittery definition for legacy module

1092de2

Rather than copying the definition file, re-export it from a committed `legacy.d.ts`. Rename `index.d.ts` so the definition can be imported.

Use individual strictness compiler options

ad891fa

--strict means future minor releases may end up working stricter in a non-backwards compatible manner.

Clean up file inclusion

445c12e

* allowJs was superfluous with the explicit include option * The include option matches the default behavior (without allowJs) * The exclude option can be simplified

Add additional compiler options

0cbb56f

Reformat tsconfig.json

73fb721

Update @types/node

aff3fc4

novemberborn added 8 commits January 2, 2018 16:13

Install ts-node and select it through npx in TS example

8610919

Clean up clock examples

9612513

Improve usability of "mapped" interface

e042d55

Create a `Typed` subclass of `Emittery` onto which we can hang the TypeScript method overloads.

Reword off() and offAny() descriptions

7a45a09

I found "unsubscribe to an event" too ambiguous.

Update TS code comments

653f921

* Mirror the README * Remove unused JSDoc annotations (as far as VS Code and Atom are concerned) * Alias the unsubscribe function which leads to nicer type hints in VS Code and Atom

Clean up remaining examples

c79d668

Remove unused snapshot

92dd473

Clean up tests

ff0b54d

* Use glob to find files * Rely on CWD, which AVA already sets to the project root

Add TypeScript support to README

369df41

sindresorhus approved these changes Jan 4, 2018

View reviewed changes

novemberborn closed this in a413aee Jan 4, 2018

dinoboff deleted the chore/typescript branch January 4, 2018 23:12

sindresorhus mentioned this pull request Nov 24, 2020

Why is Emittery.Typed a separate class? #67

Closed


		const Emittery = require('../');

		class MyEmitter extends Emittery {}

Add TypeScript definition #17

Add TypeScript definition #17

Conversation

dinoboff commented Dec 4, 2017 • edited Loading

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinoboff Dec 9, 2017 • edited Loading

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

novemberborn commented Dec 4, 2017

Choose a reason for hiding this comment

dinoboff commented Dec 4, 2017

dinoboff commented Dec 5, 2017

novemberborn commented Dec 5, 2017

dinoboff commented Dec 9, 2017 • edited Loading

novemberborn commented Dec 11, 2017

novemberborn 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

novemberborn commented Jan 2, 2018

sindresorhus commented Jan 2, 2018

novemberborn commented Jan 3, 2018

dinoboff commented Jan 3, 2018

novemberborn commented Jan 4, 2018

novemberborn commented Jan 4, 2018

sindresorhus commented Jan 4, 2018

novemberborn commented Jan 4, 2018

sindresorhus commented Jan 4, 2018

dinoboff commented Jan 4, 2018

sindresorhus commented Jan 17, 2018

dinoboff commented Dec 4, 2017 •

edited

Loading

dinoboff Dec 9, 2017 •

edited

Loading

dinoboff commented Dec 9, 2017 •

edited

Loading