TypeScript type declarations

[andraaspar]

Hi,

I have just been noticed that you are adding TypeScript definitions to Mithril once again.

I should have probably told you, but I also wrote some TypeScript definitions for Mithril, and all of it has been part of a PR to DefinitelyTyped for some weeks now. See: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/14513

The reason I bring this up is that the mithril.d.ts that is in the repo currently, does not pass the tests I have in the PR, and I could not easily modify the tests to make them pass. This makes me think that the type defs in the Mithril repo are inferior to mine. I do not claim mine is perfect, but I think it is further along the way.

I have put a fair amount of research & testing into my type defs. Here are some of the advantages:

• Can use mithril without an import statement.
• Component state, vnode.state, and this are all the same and refactoring just works.
• Component attrs, vnode.attrs are the same, and m(Component, Attrs) enforces type safety.
• It is not a complete rewrite, but an evolution of the Mithril 0.2 type defs' latest version from the repo.
• It is documented with tests. Code has been taken directly from http://mithril.js.org/ code examples.
• I have migrated a fairly large Mithril 0.2 project to 1.0 using these type defs, and it works fine.

I would like to ask you to evaluate my Mithril type defs, and consider using them in place of what you've currently got.

Actually, I think that type definitions belong on DefinitelyTyped, and not in the Mithril repo. The reason I think so is that it's easier to decouple type defs from the project, and everyone gets to install and use what they prefer. Also, @lhorie made the point once that he does not work with TypeScript, so he'd prefer the community to maintain the defs. If this is still the case, then it may be an option to support my PR to DefinitelyTyped, and remove mithril.d.ts from the Mithril repo.

Can we have a conversation about all this before you go live with the new mithril.d.ts?

@lhorie @spacejack @pixelmike and other project members can you please comment?

[andraaspar]

My tests can be found here: https://github.com/andraaspar/DefinitelyTyped/blob/master/mithril/mithril-tests.ts

[dead-claudia]

A PR to the definition file here would be strongly preferred, although I'd consult with @spacejack over this (he's more into this than I am).

[andraaspar]

That is acceptable to me – although it may take a while to process. Would you wait with the release until it's done? Or more like, how long can you wait?

To make it simpler to discuss & demonstrate issues, I'd like to also add the tests file mentioned above to the PR. Where would be a good location to place it? Name suggestions?

[dead-claudia]

@lhorie is who controls releases, not me.

[dead-claudia]

@andraaspar

To make it simpler to discuss & demonstrate issues, I'd like to also add the tests file mentioned above to the PR. Where would be a good location to place it? Name suggestions?

I'd defer to @lhorie or @tivac on that, since I really don't know where that would go.

[pygy]

@andraaspar I agree we should join forces...

I'm a TypeScript n00b, and I don't have the perspective to judge wether it is better to have the typings here or at DefinitelyTyped. An advantage of having them here is that we can better coordinate the typings developments (and avoid the current situation). There are other typescript users that follow Mithril.

If the typings were to stay here, the tests would probably go in tests/test-typings.js (for the ospec-based JS file that loads the typescript compiler then the TS sample) and tests/test-typings.ts for your tests (or a more descriptive name if you can think of one).

At a glance I see a few issues with your typings:

• It looks like your typings don't support class and closure components (or I've missed them)

• You have the stream<T> setter return void, it should return T

• We're already using @spacejack's typings in projects, and your typings are incompatible (Component<Attrs, State> vs. Component<State, Attrs>, maybe other points; full disclosure, I'm currently working under @spacejack's supervision on a Mitrhril + TS-based app)

[andraaspar]

@pygy Thank you for testing it.

It looks like your typings don't support class and closure components (or I've missed them)

export declare namespace Foo {
    interface State {}
    interface Attrs {}
}
type Vnode = Mithril.Vnode<Foo.State, Foo.Attrs>

export class Foo implements Mithril.ComponentMethods<Foo.State, Foo.Attrs> {
    // oninit(v: Vnode) { }
    // onbeforeupdate(v: Vnode, old: Vnode) { }
    view(v: Vnode): Mithril.Children {
        return m('div')
    }
    // oncreate(v: Vnode) { }
    // onupdate(v: Vnode) { }
    // onbeforeremove(v: Vnode) { }
    // onremove(v: Vnode) { }
}

This is how you'd do it. Although it is currently discouraged by the docs, and to tell you the truth, I've migrated to a plain object based solution, which is a lot easier to type (State == const Foo == this == v.state) and is closer to the docs:

export declare namespace Foo {
    interface State {
        bar: string // Same
    }
    interface Attrs {}
}
type Vnode = Mithril.Vnode<Foo.State, Foo.Attrs>

export const Foo: Mithril.Component<Foo.State, Foo.Attrs> = {
    bar: '', // Same

    // oninit(v) { },
    // onbeforeupdate(v, old) { },
    view(v) {
        v.state.bar // Same
        this.bar // Same
        return m('div')
    },
    // oncreate(v) { },
    // onupdate(v) { },
    // onbeforeremove(v) { },
    // onremove(v) { }
}

Refactoring bar above means it gets renamed in all four cases.

This is only achievable by declaring Mithril.Component as a type. Mithril.ComponentMethods is the interface that declares the view method and inherits the lifecycle methods.

type Component<State, Attrs> = ComponentMethods<State, Attrs> & State;

You have the stream setter return void, it should return T

Thanks for pointing out! I should fix that for the PR.

We're already using @spacejack's typings in projects, and your typings are incompatible (Component<Attrs, State> vs. Component<State, Attrs>, maybe other points; full disclosure, I'm currently working under @spacejack's supervision on a Mitrhril + TS-based app)

That is why I'd need some time to refactor my stuff to make it more compatible with yours. I'm hoping to meet you at some middle ground where it's not too much work for neither you people nor us.

With components written like the above, it would be fairly easy on my side to swap the two generic parameters.

On the other hand, it'd be great if you could provide a piece of sample code that demonstrates your use.

[spacejack]

@andraaspar My repo has tests, can you try your types there and see if they all pass? I'm going to be tied up for most of the day so I won't be able to look at it right away.

[spacejack]

Just took a quick look - ignoring the component style incompatibilities for the moment, you are missing types for parts of the API, and in general are not using types as much as my definitions do, so the user does not benefit from inference as much (request, route.) Some types are incorrect or incomplete (mount, render, Children, stream, etc.) Some stream functions are missing.

Simple example, this should work but doesn't:

m.mount(root, {view: function() {}})

I'll look at component style differences later.

[spacejack]

Another problem I notice - your own tests fail if you turn on TypeScript's strictNullChecks. I think most people have that flag enabled on their projects and it looks like it would be very inconvenient to use your types with this flag.

[andraaspar]

@spacejack Thanks for your feedback. I have resolved all of those issues. I am tidying up to ensure a minimal impact on merge, and once I'm happy, I'll create a PR that you can offer further feedback on.

[spacejack]

Your Component type is declared like this:

type Component<State, Attrs> = ComponentMethods<State, Attrs> & State;

I debated using that definition when I wrote mine, however it precludes component definitions that use oninit to add state properties (i.e., a component style that does not use this.) I felt it was better to allow a user to declare their own shorthand type Component = Mithril.Component<Attrs,State> & State if they predominantly use that style.

For example, this should work but does not:

interface State {
    bar: string
}

const Foo: Mithril.Component<State,{}> = {
    oninit(vnode) {
        vnode.state.bar = 'abc'
    },
    view(vnode) {
        return m('div', vnode.state.bar)
    }
}

Otherwise, the main incompatibility is the ordering of State and Attrs. I chose Attrs first because most often components are stateless, so it seemed the more common case. A number of people have starred my types and have been using them and we've been directing people to them in issues and on gitter here, so I wouldn't want to change that ordering at this point.

Forgive me for asking, but are your types not a forked version from mine a while back? It would've been nice to get some feedback and collaborate earlier on.

[andraaspar]

@spacejack No, it's completely from scratch, later adapted to fit Mithril 0.2 for merge, and now being adapted to fit yours for merge.

Don't worry, my intent is to not break any of your existing code, as much as possible. Still, the exact part you're mentioning is a must have for proper type safety in the classic component examples on the site.

How does that Component declaration preclude oninit initialization? I think it should work just fine:

export const Foo: Mithril.Component<{}, {
        bar?: string
        baz?: number
    }> = {
    oninit(v) {
        this.bar = ''
        this.baz = 5
    },
    view(v) {
        return m('div')
    }
}

[spacejack]

For a typed, stateful component, you'd write:

interface State {bar: string}
const Foo: Mithril.Component<{},State> & State = {
    bar: '',
    oninit() {
        this.bar = 'A'
    },
    view() {
        return m('p', this.bar)
    }
}

Stateful or "fat" components are the exception not the norm, so I feel the additional type annotation work is ok in this case since your component will likely be more complex anyway.

I'll agree that making state properties optional would allow your style, however then you'll be forced to add null checks whenever you use those properties. Deferred initialization is problematic in general with non-nullables (the typescript compiler currently cannot check that non-nullables are initialized in class constructors.) So I felt that not making components intersection types by default followed the spirit of "class-like" convenience vs correctness.

The most frequent usage of state I've seen in the community is by vnode.state rather than this or by properties on the component object. I thought it best to prioritize design for that.

And as I mentioned, it's easy to add your own type: type Component<A,S> = Mithril.Component<A,S> & S if you prefer.

Maybe we could add a StatefulComponent type for convenience?

Apologies, I thought I remembered your username from a list of people who had starred/forked my repo.

[andraaspar]

OK, how about Mithril.Comp? Then legacy code will not break, and if you know what you're doing, you can type the extra 5 letters.

I've personally found TypeScript's explicit null & undefined checks to be excessive, but I guess I'm just not hardcore enough.

[andraaspar]

Removed pull request in favour of https://github.com/lhorie/mithril.js/pull/1696.