chore(TS): update typescript definitions #309

TylorS · 2016-09-01T01:15:19Z

Add typings for most/lib/disposable/*
Add typings for most/lib/scheduler/*
Add typings for most/lib/.js
Add typings for mmost/lib/sink/
Update exports to ES2015

TylorS · 2016-09-15T03:16:21Z

I was thinking about this, and I think that we should definitely do this. I realized that these typings only affect how to import types not how it ends up importing the library. :D

briancavalier · 2016-09-16T11:10:57Z

@TylorS Yeah, I'm for it. What's the current status here? Do we just need to do another round of review before merging, or is there more work to be done?

TylorS · 2016-09-16T15:39:12Z

If any one is up for reviewing it, I think it's in good enough shape.

TylorS · 2016-09-16T15:41:13Z

Actually, immediately after posting that, I realized something. most/lib can be defined here as having commonjs modules, and (sadly duplicated) most/src can be defined here with ES modules

TylorS · 2016-09-16T19:15:23Z

Updated to support Es6 and Es5 imports and exports

brandonpayton · 2016-09-23T05:50:33Z

Assuming it would help, I'm going to try to get time to review this tomorrow. One thing I noticed tonight is that the latest commit switched indentation from spaces to tabs.

TylorS · 2016-09-23T06:38:08Z

Hmmm that wasn't on purpose, but I don't know if that really matters?

briancavalier · 2016-09-23T15:03:13Z

I think the only reason it matters is that the src dir is using spaces. It'd be nice to be consistent.

brandonpayton · 2016-09-26T08:13:53Z

I don't have too much further to go on the review but need to call it a night. I'll get back to this as soon as I can. So far, I have thirteen comments, mostly useful.

brandonpayton

Hi @TylorS, I hadn't looked at our type defs in depth before and did a full review. I'm sure there are things I mentioned that aren't specifically part of this PR, but I hope that is ultimately more helpful than annoying.

Thanks for putting these together.

brandonpayton · 2016-09-26T07:12:54Z

type-definitions/most.d.ts

-
-export function multicast<A>(s: Stream<A>): Stream<A>;
+declare module "most" {
+	type SeedValue<S, V> = { seed: S, value: V };


Indentation should be two spaces for the sake of consistency.
(Already mentioned, just including it with the review)

brandonpayton · 2016-09-26T07:14:57Z

type-definitions/most.d.ts

+	export function periodic<A>(period: number, a?: A): Stream<A>;
+	export function fromEvent(event: string, target: any, useCapture?: boolean): Stream<Event>;
+
+	export function unfold<A, B, S>(f: (seed: S) => SeedValue<S, B|Promise<B>>, seed: S): Stream<B>;


The documentation for unfold says the unfolding function may return a promise for a tuple, rather than a tuple with a promised value. If the documentation is correct, the declaration should be something like:

export function unfold<A, B, S>(f: (seed: S) => SeedValue<S, B>|Promise<SeedValue<S, B>>, seed: S): Stream<B>;

brandonpayton · 2016-09-26T07:15:40Z

type-definitions/most.d.ts

+
+	// es5 does not have typings for this
+	// use any to allow type-casting
+	type Generator<A, B, C> = any


What is the purpose of the B and C type params for Generator<A, B, C>?

I don't know :)

brandonpayton · 2016-09-26T07:17:26Z

type-definitions/most.d.ts

+	export default ClockTimer;
+}
+
+declare module "most/lib/scheduler/ClockTimer" {


It's cool you're supporting use of both ES5 and ES6 modules. Thank you!

brandonpayton · 2016-09-26T07:17:58Z

type-definitions/most.d.ts

+declare module "most/src/iterable" {
+	// es5 doesn't have typings for an Iterable
+	// use any to allow type-casting
+	type Iterable = any;


Should the "most" declarations import Iterable from "most/src/iterable" or vice versa? It seems like we should not have two declarations, even if they are just any for now.

I'm wondering if it's better to not include it and allow users to include them themselves like we do with Promise

That seems reasonable to me.

brandonpayton · 2016-09-27T03:51:48Z

type-definitions/most.d.ts

+		constructor(run: (t: number, x: T, sink: Sink<T>) => any, value: T, sink: Sink<T>);
+		static event<T>(x: T, sink: Sink<T>): PropagateTask<T>;
+		static end<T>(x: T, sink: Sink<T>): PropagateTask<T>;
+		static error(err: Error, sink: Sink<any>): PropagateTask<any>;


Why drop the type param and move to any for static error?

I think TypeScript is poor in handling this, but when you go to write anything with this you can never expect any type of value but an error to be propagated, however to satisfy interfaces you will have to write .event and .end on a Sink and I felt it was poor to force any types there.

Ah, that makes sense. Maybe we should add a short comment explaining why.

Let's go with any and add a comment about it.

brandonpayton · 2016-09-27T04:51:37Z

type-definitions/most.d.ts

+	import { Disposable, Sink } from 'most';
+	import SettableDisposable = require('most/src/disposable/SettableDisposable');
+
+	export function tryDispose<T> (t: number, disposable: Disposable<T>, sink: Sink<T>): Promise<T> | void;


It looks like the implementation can return a non-Promise value.

brandonpayton · 2016-09-27T05:28:19Z

type-definitions/most.d.ts

+		<A, B>(f: (a: A) => B, args: [A]): B;
+		<A, B, C>(f: (a: A, b: B) => C, args: [A, B]): C;
+		<A, B, C, D>(f: (a: A, b: B, c: C) => D, args: [A, B, C]): D;
+		<A, B, C, D, E>(f: (a: A, b: B, c: C, d: D) => E, args: [A, B, C, D]): E;


This is used by combine, and our def for that has one overload that allows for five arguments, but the largest we define for invoke allows just four arguments. Maybe we should add an invoke overload with five arguments.

Well it is possible to n number of arguments as the default case of invoke falls back to fn.apply(...)

brandonpayton · 2016-09-27T05:46:23Z

type-definitions/most.d.ts

+	type Iterable = any;
+
+	export function isIterable(x: any): boolean;
+	export function getIterator(x: any): Iterable;


The return type should be an Iterator type rather than Iterable.

brandonpayton · 2016-09-27T06:06:13Z

type-definitions/most.d.ts

+		add (x: T): void;
+		remove (x: T): void;
+		isEmpty(): boolean;
+		dispose(): Promise<T>;


This doesn't seem quite right. The implementation can return a Promise with a resolved value of nothing or an array.

briancavalier · 2016-09-27T12:28:05Z

Thanks for the very thorough review, @brandonpayton!

Add typings for most/lib/disposable/* Add typings for most/lib/scheduler/* Add typings for most/lib/*.js Add typings for mmost/lib/sink/* Update exports to ES2015

ES6 imports are used from the ES6 source code in `most/src` ES5 imports are user as before from `most/lib`

brandonpayton · 2016-11-21T02:30:45Z

Hi @TylorS, I'm sorry it's taken a while to respond to your updates. I took a look at the latest, and it looks good to me with the exception of the following:

The ap definition still appears incorrect against the implementation. Comment here.
Consider adding a comment explaining why constraints are loosened. Comment here.
Update the Timer interface discussed here. Perhaps adding a simple Asap handle type would be sufficient.

Thanks!

briancavalier · 2016-12-27T18:53:22Z

@TylorS @brandonpayton What's the current status here? Master is ahead by a good bit now, so just wondering what the next steps here need to be. Thanks!

brandonpayton · 2016-12-27T23:11:10Z

@briancavalier I believe we just need updates in response to these three items.

briancavalier · 2016-12-28T15:50:50Z

Thanks @brandonpayton. I'll do my best to reply to each of those at their original comment site.

briancavalier · 2017-04-20T11:33:07Z

Looking back at the 3 remaining items, I think we've covered them in mostjs/core. Personally, I'd like to focus on getting a 1.0 release of that, and then "fixing" the type definitions here by porting most.js to use mostjs/core and updating most.js type definitions in the process.

How does that sound?

TylorS · 2017-04-20T11:43:27Z

I agree!

brandonpayton · 2017-04-20T15:35:57Z

Sounds good to me!

TylorS · 2017-11-11T15:24:26Z

With @most/core v1 released now, I don't see a need to keep this PR open. Further issues with types will likely be better sent in that direction.

Thanks for all the hard work here @brandonpayton, the work we did here definitely did not go to waste. Much of it made it over to @most/* packages :)

brandonpayton suggested changes Sep 27, 2016

View reviewed changes

TylorS added 4 commits October 7, 2016 15:49

chore(TS): update typescript definitions

a897ad6

Add typings for most/lib/disposable/* Add typings for most/lib/scheduler/* Add typings for most/lib/*.js Add typings for mmost/lib/sink/* Update exports to ES2015

chore(TS): simultaneously support ES6 and ES5

1f189fa

ES6 imports are used from the ES6 source code in `most/src` ES5 imports are user as before from `most/lib`

chore(TS): fixes for type definitions

49db77f

chore(TS): fix SettableDisposable import

0b6140c

TylorS force-pushed the es2015-ts-defs branch from 0803457 to 0b6140c Compare October 7, 2016 19:49

TylorS closed this Nov 11, 2017

chore(TS): update typescript definitions #309

chore(TS): update typescript definitions #309

Conversation

TylorS commented Sep 1, 2016

TylorS commented Sep 15, 2016

briancavalier commented Sep 16, 2016

TylorS commented Sep 16, 2016

TylorS commented Sep 16, 2016

TylorS commented Sep 16, 2016 • edited

brandonpayton commented Sep 23, 2016 • edited

TylorS commented Sep 23, 2016

briancavalier commented Sep 23, 2016

brandonpayton commented Sep 26, 2016

brandonpayton 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

brandonpayton Sep 26, 2016 • 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

brandonpayton Sep 27, 2016 • 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

Choose a reason for hiding this comment

briancavalier commented Sep 27, 2016

brandonpayton commented Nov 21, 2016 • edited

briancavalier commented Dec 27, 2016

brandonpayton commented Dec 27, 2016

briancavalier commented Dec 28, 2016

briancavalier commented Apr 20, 2017

TylorS commented Apr 20, 2017

brandonpayton commented Apr 20, 2017

TylorS commented Nov 11, 2017

TylorS commented Sep 16, 2016 •

edited

brandonpayton commented Sep 23, 2016 •

edited

brandonpayton Sep 26, 2016 •

edited

brandonpayton Sep 27, 2016 •

edited

brandonpayton commented Nov 21, 2016 •

edited