Add Typing (DTS file) #311

Swizz · 2017-07-21T20:31:42Z

This follows and closes #279

The first attemps of a DTS file was made with a lot of stuffs in minds.
I chosen to Revoke completely my last attempt, to propose a more sanitized one.
The main goal of this PR is now to move with iteration. To easily discuss about declarations.

First, I propose you to talk about the basics, and the VNode, h() stuffs.

This declaration file, is 98% based on the Picodom one by @pspeter3 because the Vdom system is (almost) the same.

Here I have only overloaded h() and added JSX support. jorgebucaran/superfine#17 (comment)

I have also added the Emit interface and app function as starting point.

Bonus : Emit use @andrewiggins awesome `keyof Events` :

Steps

Step 1 - Virtual DOM + JSX + Emit
Step 2 - View + Actions
Step 3 - Events
Step 4 - Application + Mixin

codecov · 2017-07-21T20:36:19Z

Codecov Report

Merging #311 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #311   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         148    148           
  Branches       46     46           
=====================================
  Hits          148    148

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd51e18...4862023. Read the comment docs.

jorgebucaran · 2017-07-22T04:28:43Z

hyperapp.d.ts

+  data?: Data,
+  ...children: VElement<{}>[]
+): VNode<Data>
+


@Swizz Why is this repeated?

@Swizz Is there any way to declare that using the same definition? If it's impossible, then can we leave it out? The fact that children can be arguments is an implementation detail needed by JSX, but users should definitely not use h like that.

I am looking for a way to express children could be an array ov VElements or the rest of args could be too.

In Snabbdom-pragma, I am using the following

type Children = VNode<{}>[] | VNode<{}> | string | number type CircularChildren = Children | Children[] export function h<Data>( tag: Component<Data> | string, data?: Data, ...children: CircularChildren ): VNode<Data>

CircularChildren was about a quick fix regarding :

const arr = [ [<div>Hello1.1</div>, <div>Hello1.2</div>], [<div>Hello2.1</div>, <div>Hello2.2</div>] ] const vnode = <div> { arr } </div>

@Swizz Can we just leave out the ...children? h() supports ...children just because of JSX. If JSX didn't exist, h() would be always Array<VirtualNode>.

One more thing.
Can I use h() with a number as children ?

plus: do you prefer the Array<Data> notation instead of the Data[] one ?

I didn't know we could use both, but let's go on with Array<Data>.

Can I use h() with a number as children ?

Yes, you can.

Only in h() or we can tell a VirtualElement could be either a VirtualNode, a string, or a number.

The last one will affect :

h() children parameter

Component children

VirtualNode children

Hum.. Array<Data> could be a little bit confusing.

h( children: Array<VirtualElement<{}>> )

h( children: VirtualElement<{}>[] )

@Swizz Why don't you just repeat VirtualNode | string everywhere?

Do you prefer ?

I think is more correct to tell once a VirtualNode is ... and then the user will be mind this.
And IntelliSens will yield about the right type when aliased. It still asking for number, string, or VirtualNode. So this is just more DRY.

jorgebucaran · 2017-07-22T04:30:30Z

Great work @Swizz! Can we work on renaming a few things?

I was thinking instead of VNode, better VirtualNode.

And add types for State, Actions, Mixins, etc.

Note that actions allow nested actions so in flow:

{
  [action: string]:
    | Actions
    | (State, Actions, any) => ActionResult
}

Does that make sense?

We'll also need an ActionResult type:

A partial state or Promise that resolves to a partial state.

Maybe also a definition for CustomEvent:

event(State, Actions, any): any

jorgebucaran · 2017-07-22T04:39:02Z

hyperapp.d.ts

+
+// VNode part
+
+export interface VNode<Data> {


@Swizz Why does VNode<Data> mean?

According to @pspeter3 DTS for Picodom, I am pretty sure this is to allow to define the Props interface of a vnode. And to help for JSX 🎉

(VS Code is awesome regarding TypeScript)

Hmm, so this is only useful if the user defines an interface for the VirtualNode data? Hmm. Nice.

Yes using :

const link = h('a', {})

Will do not raise anything. Thats why we are using VNode<{}> as default. Instead of any.

jorgebucaran · 2017-07-22T04:39:18Z

hyperapp.d.ts

+  children: VElement<{}>[]
+}
+
+export type VElement<Data> = VNode<Data> | string


@Swizz Is this possible:

export interface VNode<Data> { tag: string data?: Data children: VNode<{}>[] } | string

You want a VNode to be either a VNode or a string. I never heard it was possible. Union is only possible on type.

I guess, it is right to use a type.
A VirtualNode is a VirtualNode and a VirtualElement could be either a VirtualNode or a string.

I was just trying to remove the VirtualElement type, which is just confusing.

Not if you are thinking like the last sentence.

You cant tell a VirtualNode could be a VirtualNode or a string, isnt it ?
But its easy to reason about a VirtualNode and a VirtualElement that could be either a VirtualNode or a string.

The VirtualNode is the vdom thing. And the VirtualElement is in most case a children. And a children could really be another VirtualNode or a string.

Well, if it's always the children, why not call it VirtualNodeChild?

@Swizz ☝️

Now it is used as child, but I dont think it is good to limit it at this role by naming.
I do really think VirtualElement is good. @pspeter3, did an awesome job there.

Swizz · 2017-07-22T06:38:16Z

@jbucaran

Great work @Swizz!

All credits goes to @pspeter3, seriously

Can we work on renaming a few things? I was thinking instead of VNode, better VirtualNode.

Naming is open to discussion. I am just a bit curious, why is this a concern here and not in Picodom ?

And add types for State, Actions, Mixins, etc.

Dont worry about it. As the DTS will grow faster, I want to move side by side. Now I want to discuss about VNode, JSX, and the app basis. Once done, I will move forward.
Because there are a lot of thing to discuss. So I think is better to be scope minimal.

jorgebucaran · 2017-07-22T06:40:22Z

Sounds good.

VNnode → VirtualNode
JSX 👍

Why is this a concern here and not in picodom?

I didn't say picodom anywhere.

EDIT: I do plan to change it in picodom as well.

Swizz · 2017-07-22T07:50:17Z

@jbucaran A giant plus DTS will be help to yield and suggest about types for VS Code user and atom-typescript user.
Even if they are using JavaScript instead of TypeScript.

So I plan to add JSDoc too.

jorgebucaran · 2017-07-22T09:05:47Z

hyperapp.d.ts

+export function h<Data>(
+  tag: Component<Data> | string,
+  data?: Data,
+  children?: VirtualElement<{}>[] | VirtualElement<{}> | number


@Swizz Can you make this just:

VirtualElement<{}>[]?

People using h should use its best form (string, data, array of virtualnode).

Without it TSC will yield an exception if I want to use h() that way

h('div', null, h('span', null, 'Hi.'))

This simple thing will not be possible to

h('div', null, 'Hi.') // ask for h('div', null, ['Hi.'])

Do you really want to remove features by typing ?

@Swizz You should not use it that way IMO.

@lukejacksonn What do you think?

As a compromise, one option is to add a JSDoc @deprecated comment to steer people away from this function, saying it only exists for JSX support and to use the array version for better perf.

@andrewiggins Is the function info in vscode intellisense picked up from JSDoc comments?

Correct, it is. Screenshot below shows an example of what a user would see for the argument spread version of h:

This screenshot shows an example of what a user would see for the array version of h:

I dont know how this is a problem in fact. Performance ?

You are saying h('div', null, 'Hi.') is less performant than h('div', null, ['Hi.']) ?
And what if I still want to use this feature like way, I will get typing/editor error/warning even if the function allow the use of it ?
I think this is very effortless to types it. And let users to choose you prefered way to do as both exist.

Let's write a performance warning into the doc and refer it in the JSdoc.

/** Description... * @see {@link http://github.com/hyperapp/hyperapp/... | Performance issue} */ h(....

Swizz · 2017-07-24T21:15:43Z

Discussed changes pushed, plus JSDoc !

Let's go to the Step 2 !

andrewiggins · 2017-07-25T01:18:56Z

hyperapp.d.ts

+
+/** @namespace [Virtual DOM] */
+
+/** The Virtual DOM representation of an Element


In all the examples I've seen (and on the JSDoc website itself) it appears the first line of a multi-line doc comment goes on the line after /**. It appears JavaDoc (which I assume JSDoc is inspired from) follows the same styling convention.

Not a big deal, but just thought I'd point it out.

It is my first time with JSDoc to be honest. I have let VSCode guide me with

andrewiggins · 2017-07-25T01:45:29Z

hyperapp.d.ts

+export interface VirtualNode<Data> {
+  tag: string
+  data: Data
+  children: VirtualNodeChild<{} | null>[]


What's the reasoning for adding | null here?

In strict mode null need to be ensured, this is prerequisite if you are using : h('tag', null) node

We could add a comment about that

andrewiggins · 2017-07-25T01:49:24Z

hyperapp.d.ts

+  (data?: Data, ...children: VirtualNodeChild<{} | null>[]): VirtualNode<Data>
+}
+
+/** The soft way to create a Virtual Node


What does "soft" mean? I would just state that h creates VirtualNodes

I was looking for a way to indicate "you can create VirtualNode without h too, but using h is the prefered way".

Virtual Node Factory?

andrewiggins · 2017-07-25T01:57:26Z

hyperapp.d.ts

+*/
+export type VirtualNodeChild<Data> = VirtualNode<Data> | string
+
+/** A Component is a function that return a custom Virtual Node


I'd put this comment inside the interface, on top of the method definition, and add param documentation. It provides better intellisense in VSCode

You are right, I did it for Emit.

andrewiggins · 2017-07-25T01:59:49Z

hyperapp.d.ts

+ * @memberOf [Virtual DOM]
+*/
+export interface Component<Data> {
+  (data?: Data, ...children: VirtualNodeChild<{} | null>[]): VirtualNode<Data>


I don't think the children args are spread out like this. I believe you just get a direct Array.

Already fixed I missed to push this

andrewiggins · 2017-07-25T02:02:33Z

hyperapp.d.ts

+
+/** @namespace [Application] */
+
+export interface Emit<Data, Events> {


Should we swap the order of these generic parameters to match the order of the function parameters?

Is there a way to make a Generic parameter optional ? Because we need to give generic a type even if data is optional 😕

Yup! You can specify a default type, which makes that generic param optional. For example:

interface SomeInterface<T, U=any>

Awesome thank you !

As a general rule, prefer {} to any. Using any allows the consumer to call any property/method for U vs {} will allow the consumer to call none.

andrewiggins · 2017-07-25T02:05:23Z

To maintain the quality of these typings over time I recommend we add some typings tests to the tests/ folder similar to what I did for picodom. It also makes it clearer to understand how to use the typings appropriately

Swizz · 2017-07-25T07:47:46Z

Thank you for your review @andrewiggins. Your help is a giant plus 👍

To be honest this is 50% laziness. But are you okay to write tests ?
It will ensure typing is understandable and intuitive. And this will help me a lot.

andrewiggins · 2017-07-25T16:37:54Z

Absolutely! I'll copy what I started in picodom and expand it to include hyperapp features

pspeter3 · 2017-07-26T21:04:34Z

hyperapp.d.ts

+  children?:
+    | VirtualNodeChild<{} | null>[]
+    | VirtualNodeChild<{} | null>
+    | number


Are numbers converted to strings?

I think so.. https://github.com/hyperapp/hyperapp/blob/master/src/h.js#L16

Yes, a h children can be a number, and number will be converted to a string in the VirtualNode representation.

h also supports array of numbers (all converted to strings) should it then be:

children?: Array<number | VirtualNodeChild<{} | null>> | VirtualNodeChild<{} | null> | number

pspeter3 · 2017-07-27T21:32:09Z

After we merge this, doing one for the router would be great.

Swizz · 2017-07-28T07:41:01Z

DTS typing of Hyperapp is a Masterpiece. I think typing the Router will be a lot more easy.

I am a little bit busy these days. I hope I'll push the step 2, within a week.

Swizz · 2017-07-28T08:45:46Z

@pspeter3 Is there a way to ensure all api function to implements ... ?

export interface API<State, Actions, DataIn, DataOut> {
    (state: State, actions: Actions, data: DataIn): DataOut
}

pspeter3 · 2017-07-28T17:35:08Z

@Swizz what API functions are you talking about?

jorgebucaran · 2017-08-08T20:45:23Z

@Swizz @pspeter3 So #317 was merged! We can now move forward with this. 👍

See: https://github.com/hyperapp/hyperapp/blob/master/docs/api.md

mindplay-dk · 2017-08-18T09:31:22Z

How can I install and try this?

jorgebucaran · 2017-08-18T09:32:31Z

Pong @Swizz @pspeter3.

Swizz · 2017-08-18T09:37:14Z

Quickest way :

In Hyperapp repo

git checkout -b Swizz-fresh-typing master
git pull git://github.com/Swizz/hyperapp.git fresh-typing
npm link

Or clone my repository

git clone -b fresh-typing git://github.com/Swizz/hyperapp.git hyperapp
cd hyperapp
npm link

In you demo project, instead of installing hyperapp release use your local one

npm link hyperapp

mindplay-dk · 2017-08-18T10:56:16Z

Thanks @Swizz but I'd rather not use links, as these wouldn't work for anyone else cloning the project. (would they?)

According to npm docs, this should work:

"dependencies": {
  "hyperapp": "git://github.com/Swizz/hyperapp.git#fresh-typing",

It even displays the expected hash 93326df on the console during installation - yet, I don't know what it's installing or why, but there are no typings in the installation folder afterwards.

Swizz · 2017-08-18T11:13:02Z

To be honest, I always using in current dev DTS that way.
I know about /// <reference path="..." /> but I never found how to use it.
Maybe @andrewiggins or @pspeter3 could help.

pspeter3 · 2017-08-18T18:05:28Z

They would not work. Using the reference path is an outdated way to tell the compiler about type definitions. You can now using the typings key in tsconfig.

jorgebucaran · 2017-08-18T18:40:07Z

@Swizz Can you please update this with the new API? :)

Swizz · 2017-08-19T10:03:38Z

Now synced with 0.12.0 🎉

jorgebucaran · 2017-08-25T18:45:17Z

@Swizz What else do we need to merge this?

okwolf · 2017-09-01T04:10:44Z

hyperapp.d.ts

+
+export function app<State, Actions, Events>(app: {}): Emit<{} | null, Events>
+
+/** @namespace [JSX] */


Why is there JSX typing in here? I didn't think we implement any of that within Hyperapp.

It's for the TypeScript compiler. It uses that to type check JSX when generating the equivalent JavaScript

okwolf · 2017-09-01T04:20:16Z

hyperapp.d.ts

+export type ActionsResult<State> =
+  | Partial<State>
+  | Promise<Partial<State>>
+  | Function // /!\ TODO: Type the thunk Function


Mission Control: I believe we need a Thunk type.

okwolf · 2017-09-01T04:21:22Z

hyperapp.d.ts

+}
+
+export interface View<State, Actions> {
+  (state: State, actions: Actions): VirtualNode<{}>


Might want to add a comment describing this.

api.md will be useless after #311.

- Remove api.md and display type information next to topic. - Improve docs. - See also #311.

Swizz

Nevermind, Emit copy pasta 😆

Swizz · 2017-09-07T09:01:51Z

You can now, please, review the View and Actions part

jorgebucaran · 2017-09-07T16:06:26Z

hyperapp.d.ts

+  state?: State
+  actions?: Actions
+  events?: Events
+  view?: View<State, Actions>


@Swizz what about root and mixins?

This is only minimal, for testing state, actions, events and view. Application typing will come later

jorgebucaran · 2017-09-07T16:07:33Z

hyperapp.d.ts

+export interface Events<
+  State extends Hyperapp.State,
+  Actions extends Hyperapp.Actions<State>
+> {}


@Swizz What about an event third data argument?

This is not arguments but the generics. Here is no interface for Events yet

jorgebucaran · 2017-09-07T16:09:11Z

hyperapp.d.ts

+  | void
+
+
+export type ActionsCallers<


@Swizz What if we call these VirtualActions?

/cc @andyrj @pspeter3

For naming I'll let you choose

pspeter3 · 2017-09-07T23:15:06Z

What can I help with here?

okwolf · 2017-09-08T00:26:03Z

hyperapp.d.ts

+   *
+   * @memberOf [Virtual DOM]
+  */
+  (data: Data, children: VirtualNodeChild<{} | null>[]): VirtualNode<Data>


React and Vue.js refer to data as props, should we follow the same convention?

Could be a good idea! We'd need to change it in app and h too.

Just to be complete, Angular refers to it as inputs, although in Angular's case, it's impossible for an input not to also become state inside the component after it's received.

I'm also for renaming, especially since in Vue.js data refers to state inside the component.

Give the names you want to. I am really bad at naming stuff. I followed the API.md

@Swizz Looks good, but what about the tests? We can't merge without tests.

@andrewiggins Are you always among us? Ready to help me about tests?

I'm here :) Checkout what I did in jorgebucaran/superfine#19 if you want a simple example of some tests. In summary, add some typescript files that demonstrates how you would expect the types to be used, and add a call to tsc in the test npm script to make sure they compile correctly.

jorgebucaran · 2017-09-17T20:47:50Z

@Swizz Is this ready? I'm ready to take over now. 😄

Swizz · 2017-09-18T09:13:30Z

There is a lot of commits...
Did I fail my rebase ?

pspeter3 · 2017-09-18T21:00:48Z

🎆

jorgebucaran · 2017-09-18T21:02:05Z

Merged, merged, merged! 💥 🎉

- Remove api.md and display type information next to topic. - Improve docs. - See also #311.

Swizz mentioned this pull request Jul 21, 2017

Add TypeScript declaration file #279

Closed

jorgebucaran reviewed Jul 22, 2017

View reviewed changes

jorgebucaran requested changes Jul 22, 2017

View reviewed changes

andrewiggins mentioned this pull request Jul 24, 2017

Improve TypeScript types and add TypeScript compilation tests jorgebucaran/superfine#19

Merged

andrewiggins reviewed Jul 25, 2017

View reviewed changes

pspeter3 reviewed Jul 26, 2017

View reviewed changes

jorgebucaran mentioned this pull request Aug 15, 2017

Road to 1.0 #244

Closed

7 tasks

okwolf reviewed Sep 1, 2017

View reviewed changes

jorgebucaran pushed a commit that referenced this pull request Sep 1, 2017

Remove api.md and display type information next to topic.

c19dfec

api.md will be useless after #311.

jorgebucaran mentioned this pull request Sep 1, 2017

Remove api.md and display type information next to topics. #348

Merged

jorgebucaran pushed a commit that referenced this pull request Sep 1, 2017

Remove api.md and display type information next to topic.

1659f46

api.md will be useless after #311.

jorgebucaran pushed a commit that referenced this pull request Sep 3, 2017

Remove api.md and display type information next to topic.

d7620e1

api.md will be useless after #311.

jorgebucaran pushed a commit that referenced this pull request Sep 3, 2017

Remove api.md and display type information next to topics. (#348)

7e4f722

- Remove api.md and display type information next to topic. - Improve docs. - See also #311.

Swizz commented Sep 5, 2017

View reviewed changes

jorgebucaran reviewed Sep 7, 2017

View reviewed changes

okwolf reviewed Sep 8, 2017

View reviewed changes

Swizz force-pushed the fresh-typing branch 3 times, most recently from 8010168 to 2db944f Compare September 18, 2017 12:51

Add hyperapp.d.ts declaration file

4862023

Swizz force-pushed the fresh-typing branch from 2db944f to 4862023 Compare September 18, 2017 12:59

jorgebucaran merged commit d6322fc into jorgebucaran:master Sep 18, 2017

jorgebucaran added the enhancement New feature or request label Sep 18, 2017

Swizz deleted the fresh-typing branch December 26, 2017 15:06

jorgebucaran added a commit that referenced this pull request Jan 7, 2018

Remove api.md and display type information next to topics. (#348)

7817c8e

- Remove api.md and display type information next to topic. - Improve docs. - See also #311.

jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018

Add hyperapp.d.ts declaration file (#311)

2ac5872


		/** @namespace [Virtual DOM] */

		/** The Virtual DOM representation of an Element


		/** @namespace [Application] */

		export interface Emit<Data, Events> {


		export function app<State, Actions, Events>(app: {}): Emit<{} \| null, Events>

		/** @namespace [JSX] */

Add Typing (DTS file) #311

Add Typing (DTS file) #311

Conversation

Swizz commented Jul 21, 2017 • edited Loading

Steps

codecov bot commented Jul 21, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgebucaran Jul 22, 2017 • edited Loading

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

Swizz Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

jorgebucaran commented Jul 22, 2017 • edited Loading

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

jorgebucaran Jul 22, 2017 • edited Loading

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

Swizz Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

Swizz commented Jul 22, 2017 • edited Loading

jorgebucaran commented Jul 22, 2017 • edited Loading

Swizz commented Jul 22, 2017 • edited Loading

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

Swizz commented Jul 24, 2017

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

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

Choose a reason for hiding this comment

andrewiggins commented Jul 25, 2017

Swizz commented Jul 25, 2017

andrewiggins commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mytrill Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

pspeter3 commented Jul 27, 2017

Swizz commented Jul 28, 2017

Swizz commented Jul 28, 2017

pspeter3 commented Jul 28, 2017

jorgebucaran commented Aug 8, 2017

mindplay-dk commented Aug 18, 2017

jorgebucaran commented Aug 18, 2017

Swizz commented Aug 18, 2017

mindplay-dk commented Aug 18, 2017

Swizz commented Aug 18, 2017

Swizz commented Jul 21, 2017 •

edited

Loading

codecov bot commented Jul 21, 2017 •

edited

Loading

jorgebucaran Jul 22, 2017 •

edited

Loading

Swizz Jul 22, 2017 •

edited

Loading

jorgebucaran commented Jul 22, 2017 •

edited

Loading

jorgebucaran Jul 22, 2017 •

edited

Loading

Swizz Jul 22, 2017 •

edited

Loading

Swizz commented Jul 22, 2017 •

edited

Loading

jorgebucaran commented Jul 22, 2017 •

edited

Loading

Swizz commented Jul 22, 2017 •

edited

Loading

Mytrill Sep 17, 2017 •

edited

Loading

jorgebucaran commented Aug 25, 2017 •

edited

Loading

Swizz commented Sep 7, 2017 •

edited

Loading