#1686: Updated TypeScript defs.

[andraaspar]

• Added type checked m(Component, Attrs) def.
• Added Comp as better type checked alternative to Component.
• Refactored VnodeDOM into Vnode.
  This was overly limiting. It prevented use like this:

export const Foo: Mithril.Component<{}, {}> = {
	view(v) {
		return (
			m('button', {
				'type': 'button',
				'onclick': () => {
					jQuery(v.dom).trigger('close')
				}
			},
				'OK'
			)
		)
	}
}

• Simplified various types into ComponentTypes.
• Renamed A, S => Attrs, State
• Minor fixes.
• Added missing bits.
• Added comments.
• Formatted.
• Added tests.

[andraaspar]

Suggestions for fixing CI?

[pygy]

It was a flaky async test. Restarting Travis fixed it.

[andraaspar]

Reverted removing VnodeDOM. It was silly.

[spacejack]

@andraaspar thanks, I think this looks really good at first glance. Passes all my own tests but have yet to try it on my current projects.

Sorry about the Component definition, I struggled with that back and forth myself and in fact will likely use your type as that's my preferred style. However using vnode.state is really common in the community and strictNullChecks is always the recommended (if not most used?) setting for TS so it seemed a bit more flexible.

I really won't have time to go through all the changes for a day or two but a couple notes for now:

Hyperscript.fragment(attrs: Lifecycle<any, any> & { [key: string]: any }, children: Children): Vnode<any, any>;

I think children must be an array so I think the type should be children: Children[]

For Vnodes, I'm not entirely sure what a Vnode.children type should be. One of mithril's own tests uses vnode.children.length so I'm not sure if it's always an array or was just known to be in that test.

Thanks for catching a variety of other things like the RouteResolver this context. I'm definitely glad to have another pair of eyes on the types.

@isiahmeadows any other comments?

[pygy]

for vnodes, from the docs:

property	type	description
children	`(Array	String

I don't know if there's a way to define a type for T extends Components | (all strings but "<" and "#")...

[andraaspar]

@spacejack Thank you for the feedback.

According to the docs, the children argument of m.fragment accepts Array<Vnode>|String|Number|Boolean. According to my tests, all of this is OK except a string. Maybe @lhorie could clarify? I have modified the signature accordingly.

[andraaspar]

@pygy I have fixed vnode.children. It now uses the same type as m.fragment to keep it DRY.

I don't know if there's a way to define a type for T extends Components | (all strings but "<" and "#")...

There should be, but maybe in a separate task.

[andraaspar]

@lhorie @spacejack One other question: JsonpOptions.background seems to be undocumented. I've kept it from @spacejack's file, as he has a test for it.

[pygy]

I just looked at the Vnode situation beyond m.vnode()... According to the docs, the only fields mandatory on vnode are tag and state, the current interface in the .d.ts file has many more mandatory fields.

(The engine tolerates receiving vnodes without state, to some extent (the only problem is for non-component vnodes with stateful hooks, which will not work if the initial vnode didn't have a state object)).

For m.vnode() there's no documentation proper at this point, it is intended for low level tools like mopt. Having single source of vnodes makes render monomorphic relative to vnodes, and leads to better perf. m() uses it under the hood and so do m.mount() and m.route() for rendering the root component.

The signature is m.vnode(tag, key?, attrs?, children?, text?, dom?): Vnode... I'm not sure how you'd handle the Vnode-the-structure vs Vnode-the-function interface distinction that would happen if the usual convention of "the capitalized function name is the interface" were to be followed...

[pygy]

@andraaspar if you define interfaces for both tag: "<", children: string | number | boolean and tag: string|Component, children: Vnode[], shouldn't TypeScript sort things out? The types are unambiguous AFAICT.

[andraaspar]

@pygy

According to the docs, the only fields mandatory on vnode are tag and state, the current interface in the .d.ts file has many more mandatory fields.

The only mandatory field that should be optional to match the docs is Vnode.attrs.

I have just tested and Mithril always provides the attrs in components. Maybe the docs need to change.

I'm not sure how you'd handle the Vnode-the-structure vs Vnode-the-function interface distinction that would happen if the usual convention of "the capitalized function name is the interface" were to be followed...

I'd suggest VnodeFactory.

if you define interfaces for both tag: "<", children: string | number | boolean and tag: string|Component, children: Vnode[], shouldn't TypeScript sort things out?

I think it could work. My only objection is that we already have Vnode, VnodeDOM, CVnode and CVNodeDOM. Introducing three subclasses will result in 12 new interfaces. Is this a feature that you use so frequently that it's worth the complexity?

[spacejack]

I don't think vnode is importable as a standalone module, so we could just have a function signature for Static.vnode rather than creating an interface.

[pygy]

@adamvlasak regarding vnode, you're right, attrs is the only optional field marked as mandatory I overlooked a few ?.

But it isn't so much what m() and m.vnode() produce, but what m.render accepts, and there are null checks for attrs (and many tests without attrs). So attrs is optional.

This made me think about the state field a bit, its default value in m.vnode() annoys @isiahmeadows and he's right, it is waste of memory... We can do better but it is a topic for another issue

[spacejack]

So one last item that was bugging me led me down a bit of a rabbit hole.

I didn't like exporting the globals, as most people would be using it as a module rather than as a script file. I looked into ways around this, and as @isiahmeadows had noted to me earlier, there is a better, more current way to write type definitions.

Instead of writing:

declare const m: Mithril.Static;

declare module 'mithril' {
	const m: Mithril.Static;
	export = m;
}

we should be doing:

declare const Mithril: Mithril.Static;
export as namespace m;
export = Mithril;

In which case, module usage would be:

import * as m from 'mithril'
import {Component, Vnode, ...} from 'mithril'

and script usage would be:

const c: m.Component<{},{}> = {view(){return m('p', 'abc')}}

Modules would not be polluted by the global definition or global namespace and both would be more succinct overall.

However... that would likely involve a lot of re-writing for everyone involved and I doubt anyone wants to be pressured into doing that right away.

So my suggestion would be to remove type definitions from mithril so it can go ahead with the next release, and we can keep using types from our own repos. Then we can pool our efforts into creating types the right way for DefinitelyTyped and reduce the noise level on the mithril.js repo.

As @andraaspar said earlier, using DefintelyTyped allows users to choose their own types.

I was personally pretty excited to have type definitions included with the framework I love, but at this point I don't think it's the right decision.

[spacejack]

Here's an example of how I think the definitions should be written:

https://github.com/spacejack/mithril.d.ts/blob/improved/index.d.ts

Stream:
https://github.com/spacejack/mithril.d.ts/tree/improved/stream

I haven't dealt with all the sub-modules yet. That would take some more work.

[andraaspar]

@spacejack I have looked into the problem and it is possible to replace the default type declarations that come with a module. You can do it using the compilerOptions.paths property in tsconfig.json, as suggested here:

{
	"compilerOptions": {
		"baseUrl": ".",
		"paths": {
			"mithril*": [
				"custom-types/my-mithril.d.ts"
			]
		}
	}
}

This means that users may choose a different type declaration even if one is shipped with Mithril by default.

What I gather from the code sample you created is that ideally, we should have one or multiple .d.ts files next to each module .js file in the Mithril repo. Would that be acceptable to project members @lhorie @isiahmeadows ?

[spacejack]

Ideally I think we want .d.ts files for all importable sub-modules, including hyperscript.d.ts, route.d.ts, request.d.ts, etc. I haven't tried this yet so I'm not 100% sure how to write those. Either way I don't think official types should be included in the next release because I don't think they'll be ready.

@lhorie and @tivac are pretty ambivalent toward TS, and I know they're pretty busy with core library stuff. I'm not sure how @pygy feels about it after getting roped into a TS project with me. :) But my gut feeling now is that types should live in DT.

[andraaspar]

Let's go with that then.

[chebum]

May I ask where is current version of .d.ts files being developed? I checked main DefinitelyTyped repo and it contains outdated typedefs. Probably, 0.xx since they still contain Component.controller definitions and according to 1.0.1 docs there is no controller property anymore.

[dead-claudia]

@andraaspar BTW, @spacejack is the one who's been the most involved in the TS definitions for the past while.

@chebum Those are massively outdated, and @spacejack only recently started looking to replace those. Historically, our definitions have lived in the repo itself, though, so until he starts rectifying that, assume the most current one is what lives in next here.

[spacejack]

@chebum you can install types from here for now if you want: https://github.com/spacejack/mithril.d.ts

[chebum]

@spacejack @isiahmeadows thank you for the input, guys.