Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TypeScript type definitions #23

Merged
merged 2 commits into from
Oct 24, 2016
Merged

Conversation

alvivi
Copy link
Contributor

@alvivi alvivi commented Oct 23, 2016

Typescript type declarations are broken, at least from typescript 2.0.3 (I have not tried other versions). This pull request makes this project usable from typescript.

@acstll acstll merged commit f3cd0cf into snabbdom:master Oct 24, 2016
@acstll
Copy link
Member

acstll commented Oct 24, 2016

Thank you @alvivi

Maybe this needs a bit more work:

https://github.com/snabbdom/snabbdom-to-html/blob/master/type-definitions/snabbdom-to-html.d.ts#L4

I'm not a TypeScript user myself so there's some syntax details I don't control. Map is the native ES2015 implementation so its arguments can vary, like being <string, number>, <string> or nothing… I think there's typings for this ES2015 stuff, maybe adding the Map one will be the most useful? What do you think?

Thanks again.

@alvivi
Copy link
Contributor Author

alvivi commented Oct 24, 2016

@acstll, yes, this needs more work. But IMHO, the problem is not with Map.

Map type declaration is bundled with Typescript. You can read the definition here. You can use Map from typescript when targeting ES6 or adding the lib "es2015.collection" and supplying a polyfill.

The Map declaration in Typescript is generic; that is why you cannot use Map without its type parameters. All type parameters must be defined to use a generic type, so you cannot use something like Map<string> or Map. Maybe the type Map<string, string> is too restrictive. Reading the code seems that the only type used for keys are strings. For value types, is the same. If there is any other possible type for values we can use something like Map<string, any> or Map<string, string | string[]>.

There are still problems using this library from Typescript. First I cannot use the default toHTML function because of this. Second, I think that the definition of VNode is not correct. However, that definition comes from Sanbbdom itself. The field sel is marked as required, but that is not true, because I can create valid VNodes with no sel, like {text: "hello world"}. This fork has a proper VNode definition.

@TylorS
Copy link
Member

TylorS commented Oct 25, 2016

@alvivi If/When you feel you've got decent typings here, feel free to ping me and I'll review them. I wrote them without ever actually using this library, but for supporting TypeScript and to do server-side rendering for (Motor)Cycle.js. snabbdom-ts is temporarily a fork I've written, but it will be mainlined in the future when I have the time.

@acstll
Copy link
Member

acstll commented Oct 25, 2016

@alvivi thanks for the explanation.

The style module can take numbers.

https://github.com/snabbdom/snabbdom-to-html/blob/master/modules/style.js#L21
https://github.com/snabbdom/snabbdom-to-html/blob/master/test/index.js#L66

and also, in theory, inside a custom module you could just call attributes(key) to get a value already in the map, and do stuff based on that, and that would be it :-)

Maybe Map<string, string|number> fits all that?

As for the problem with the VNode definition, as @TylorS suggests, there's a plan to port Snabbdom to TypeScript (snabbdom/snabbdom#150), but we don't know exactly when that will happen.

In fact, we also wanted to port this library to TypeScript (I have a branch with a first try, maybe I could push it so you can review/test it if you feel like it), but just haven't got the time to work on that.

@TylorS
Copy link
Member

TylorS commented Oct 25, 2016

@acstll I'll get into gear soon, just finished moving, and should be getting back to normal shortly :)

@acstll
Copy link
Member

acstll commented Oct 25, 2016

@TylorS no worries, I know moving takes tons of time. It will be great to have you back to "normal" 😄

@acstll acstll changed the title Fix typescript Fix TypeScript type definitions Oct 25, 2016
@acstll acstll mentioned this pull request Dec 7, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants