Typing suggestions

[dead-claudia]

I noticed you had issues with typing components, so I decided to take a look at your typings. Here's a couple suggestions of mine:

1. This m(component, attrs?, ...children) line should probably be two declarations, with a constrained generic parameter. That, I believe, should solve the parameter issue.

   // Old
   <A,S>(component: TComponent<A,S>, a?: (A & Lifecycle<A,S>) | Children, ...children: Children[]): Vnode<A,S>;
   
   // New
   (component: TComponent<any, any>, ...children: Children[]): Vnode<any, any>;
   <A extends Lifecycle<A, S>, S>(component: TComponent<A,S>, a: A, ...children: Children[]): Vnode<A,S>;

   Additionally, that constraint should probably be propagated everywhere else for the vnode, too, just for correctness.

2. It might be a good idea to introduce an interface Attrs<S> extends Lifecycle<this, S> {} to simplify the above common constraint, in the case of components especially.
3. I feel Component should be renamed AnyComponent and TComponent<A, S> renamed Component<A, S>, just to clarify which is which.
4. Most of Static's members should honestly be inlined - it'll make for a much better debugging experience.

Here's a few issues I found as well:

1. Stream#ap should really be this (although lhorie/mithril.js#1352 will likely change this later):

   // Incorrect
   ap(f: Functor<T>): Functor<T>;
   
   // Correct
   ap: <U, V>(this: Stream<(value: U) => V>, value: U) => Stream<V>;
   
   // After #1352 is resolved
   ap<U>(f: Stream<(value: T) => U>): Stream<U>;

2. m.stream.combine only accepts an array now. Also, it might be worth adding a few typed overloads.

[spacejack]

Thanks Isiah, those suggestions are really helpful. I'll try to work those changes in.

A few notes:

1: To clarify - the issue I was having was in the experimental types I wrote to accommodate the proposed class and factory components (see this repo.) Your fix still does not enable type checking for the attrs object in the hyperscript call when using a class component (example here.) I'm not sure if there is a way to do that as the parameter is a typeof which cannot be generic (see here). That said, I think yours a better definition for hyperscript so I'll use it.

3: Yeah, I wasn't sure about the naming. We'd also need names for class components and factory components once they're merged into mithril, as well as the "any" versions of those. So in total there would be: Component, AnyComponent, ClassComponent, AnyClassComponent, FactoryComponent and AnyFactoryComponent. (I wish TS allowed for a default type in a generic.) What do you think?

4: The reason I externalized those was because I was originally under the impression that 1.0 was more modular and that you could import piecemeal like import * as route from 'mithril/route'. Which would require its own module definition. But it seems that most submodules aren't actually independent, aside from hyperscript and a few of the utility functions like withAttr (see here.)

Regarding the parameter type for the callback withAttr returns, I originally used Event but couldn't compile the test-api.js when I converted to TypeScript due to this usage which needed more permissive Event and currentTarget types.

Would you like me to add you as a collaborator to this repo?

[dead-claudia]

First, thanks for the quick response. I've been really busy and highly unavailable lately, so I'm sorry if I'm only of minor help. Now, down the list:

1. I sure I can find a way to get that to be inferrable (the typeof I'm not sure will ultimately be necessary). It'll just take a bit of work (it was hard in the first place).
2. First, you could make Component<A, S> a union of ObjectComponent<A, S> | ClassComponent<A, S> | FactoryComponent<A, S>. Second, I'm not entirely sure AnyComponent adds more value over Component<any, any>.
3. That's easy enough to do - To take mithril/route as an example: export {route as default} from './mithril'. You could optionally add the relevant properties as named exports as well, but it's not entirely necessary, and is merely convenience.

Would you like me to add you as a collaborator to this repo?

Sure, I'm okay with that. Like I said earlier, I may not always be available, but I'm willing to work with you on this.

On Mon, Oct 10, 2016, 17:45 spacejack notifications@github.com wrote:

Thanks Isiah, those suggestions are really helpful. I'll try to work those changes in.

A few notes:

1: To clarify - the issue I was having was in the experimental types I wrote to accommodate the proposed class and factory components (see this repo https://github.com/spacejack/mithril-component-types.) Your fix still does not enable type checking for the attrs object in the hyperscript call when using a class component (example here https://github.com/spacejack/mithril-component-types/blob/master/src/app.ts#L28-L39.) I'm not sure if there is a way to do that as the parameter is a typeof which cannot be generic (see here https://github.com/spacejack/mithril-component-types/blob/master/src/typings/mithril.d.ts#L21 ). That said, I think yours a better definition for hyperscript so I'll use it.

3: Yeah, I wasn't sure about the naming. We'd also need names for class components and factory components once they're merged into mithril, as well as the "any" versions of those. So in total there would be: Component, AnyComponent, ClassComponent, AnyClassComponent, FactoryComponent and AnyFactoryComponent. (I wish TS allowed for a default type in a generic.) What do you think?

4: The reason I externalized those was because I was originally under the impression that 1.0 was more modular and that you could import piecemeal like import * as route from 'mithril/route'. Which would require its own module definition. But it seems that most submodules aren't actually independent, aside from hyperscript and a few of the utility functions like withAttr (see here https://github.com/spacejack/mithril.d.ts/blob/master/mithril.d.ts#L195-L203 .)

Regarding the parameter type for the callback withAttr returns, I originally used Event but couldn't compile the test-api.js when I converted to TypeScript due to this usage https://github.com/spacejack/mithril.d.ts/blob/master/test-api.ts#L87 which needed more permissive Event and currentTarget types.

Would you like me to add you as a collaborator to this repo?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spacejack/mithril.d.ts/issues/1#issuecomment-252754593, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBPiYTJ2kMQLtf35ZZssPXYrcUrmdks5qyrHcgaJpZM4KS0w2 .

[spacejack]

Good point about the any versions. I started off with untyped Vnodes, then thought it might be a convenience for simple components, but I suppose the user could write their own type in that case.

No problems being busy, I'm pretty swamped at work myself. I likely won't have time to work on this for a few days anyway.

[dead-claudia]

The reason typed vnodes (I generally prefer including the component as part of the type) are superior are so you can keep your different UI components exhaustively type checked. It adds a layer of verification you wouldn't get otherwise.

On Wed, Oct 12, 2016, 23:02 spacejack notifications@github.com wrote:

Good point about the any versions. I started off with untyped Vnodes, then thought it might be a convenience for simple components, but I suppose the user could write their own type in that case.

No problems being busy, I'm pretty swamped at work myself. I likely won't have time to work on this for a few days anyway.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spacejack/mithril.d.ts/issues/1#issuecomment-253401116, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBAMRLOWWVAQXOXTPXq_NyShRrn2rks5qzZ9OgaJpZM4KS0w2 .

[spacejack]

Going back to suggestion #1 of yours - I've had some trouble getting this to work in a convenient way, when using lifecycle methods in a hyperscript parameter (as opposed to methods on a component.)

TS complains about this line because the component comp2 does not specify the onremove lifecycle method in its Comp2Attrs type.

As it is currently, you can declare an attrs object with just the properties that your component cares about, and the hyperscript function will still recognize the lifecycle methods and infer their parameter types if you decide to use them from a parent component.

I'd like to preserve the convenience of declaring simple component types as easily as this:

const myComponent: Mithril.Component<{title: string},{}>

Otherwise, I've made some of your suggested fixes to Streams, and removed the 'any' Component type.

[dead-claudia]

TS complains about this line because the component comp2 does not specify the onremove lifecycle method in its Comp2Attrs type.

That's mildly surprising. Shouldn't all of those properties be optional? (i.e. {} being assignable to it)

I'd like to preserve the convenience of declaring simple component types as easily as this:

I also agree with you on that. I'd have to experiment quite a bit more to figure that out, though. (I've been recently more focused on rewriting half of Thallium than contributing to this project, because that framework had some fundamental design mistakes early on, but that's starting to finally begin to wrap up.)

On Fri, Oct 14, 2016, 21:43 spacejack notifications@github.com wrote:

Going back to suggestion #1 https://github.com/spacejack/mithril.d.ts/issues/1 of yours - I've had some trouble getting this to work in a convenient way, when using lifecycle methods in a hyperscript parameter (as opposed to methods on a component.)

TS complains about this line https://github.com/spacejack/mithril.d.ts/blob/master/mithril-tests.ts#L51 because the component comp2 does not specify the onremove lifecycle method in its Comp2Attrs type.

As it is currently, you can declare an attrs object with just the properties that your component cares about, and the hyperscript function will still recognize the lifecycle methods and infer their parameter types if you decide to use them from a parent component.

I'd like to preserve the convenience of declaring simple component types as easily as this:

const myComponent: Mithril.Component<{title: string},{}>

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spacejack/mithril.d.ts/issues/1#issuecomment-253954809, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBNU2OylOs2_-_Hkb2Sk1KaUmypMEks5q0C-4gaJpZM4KS0w2 .

[spacejack]

On the topic of class component types, a hyperscript signature something like this:

<A,S,C extends {new(): ClassComponent<A,S>}>(component: C, a?: (A & Lifecycle<A,S>) | Children, ...children: Children[]): Vnode<A,S>;

might get us somewhere... unfortunately I can't seem to write up a class component type that will satisfy TS.

[dead-claudia]

I'll see if I can find some time tomorrow to experiment in the TS playground. I might come up with something.

On Sat, Oct 15, 2016, 18:30 spacejack notifications@github.com wrote:

On the topic of class component types, a hyperscript signature something like this:

    <A,S,C extends {new(): ClassComponent<A,S>}>(component: C, a?: (A & Lifecycle<A,S>) | Children, ...children: Children[]): Vnode<A,S>;

might get us somewhere... unfortunately I can't seem to write up a class component type that will satisfy TS.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spacejack/mithril.d.ts/issues/1#issuecomment-254015018, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBF-FrZEAwkeY3gaFtXuEXDMKOPB1ks5q0VQAgaJpZM4KS0w2 .