Typescript support #9

joseluisq · 2018-07-07T23:39:51Z

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

frenzzy · 2018-07-08T15:50:41Z

@joseluisq thanks for the pull request! 🍻

Unfortunately this project does not use TypeScript and was written in pure JavaScript for simplicity. As a single maintainer I don't feel like I can commit to maintaining these typings over time. I appreciate the work you put into creating them to begin with, but stale typings seem inevitable as I make changes to the codebase given my lack of familiarity with TypeScript.

Please resubmit typings to DefinitelyTyped repo. I'd be happy to add a mention in the readme linking to your submission for other developers who want to find them.

joseluisq · 2018-07-08T18:06:00Z

Unfortunately this project does not use TypeScript and was written in pure JavaScript for simplicity. As a single maintainer I don't feel like I can commit to maintaining these typings over time. I appreciate the work you put into creating them to begin with, but stale typings seem inevitable as I make changes to the codebase given my lack of familiarity with TypeScript.

Sure, I understand your reasons.

Please resubmit typings to DefinitelyTyped repo. I'd be happy to add a mention in the readme linking to your submission for other developers who want to find them.

Yeah, I will try to redirect it to DefinitelyTyped because in my company we use Typescript and have chosen Hyperapp (for several reasons) and typings support. So I thought it's not a bad idea to add typings for render too.
Anyway, Thanks for the clarification.

jorgebucaran · 2018-07-09T00:10:31Z

@joseluisq I completely understand @frenzzy point of view! The good news is that a future version after V2 may include built-in support for renderToString, then I'd be happy to include your typings in the main repo.

Until then! 🙇

joseluisq · 2018-07-09T06:49:10Z

@joseluisq I completely understand @frenzzy point of view! The good news is that a future version after V2 may include built-in support for renderToString, then I'd be happy to include your typings in the main repo.

Until then! 🙇

Sounds great 👍

At the moment, I have prefered to add above render typings into my project directly.
Since Hyperapp is not into DefinitelyTyped so it is not possible to reference
/// <reference types="hyperapp" /> from other typings file like render.d.ts.
Because (in this case) withRender(nextApp) function depends on Hyperapp typings.

Complementing what it already explained on this thread, it would be great if Hyperapp could maintain typings more consistent across the @hyperapp organization in coming releases.

jorgebucaran · 2018-07-09T06:52:43Z

@joseluisq This is difficult to do because few of us actually use TS, but I'm going to try more! 💪

joseluisq · 2018-07-09T07:01:14Z

@joseluisq This is difficult to do because few of us actually use TS, but I'm going to try more! 💪

Sure, I could contribute if you need an extra hand ⚡️

frenzzy · 2018-07-09T07:04:09Z

Btw, I don't mind to add typings to this repo if you are going to maintain them in the future :)

joseluisq · 2018-07-09T07:10:22Z

Btw, I don't mind to add typings to this repo if you are going to maintain them in the future :)

Happy to hear about it! Let me know how can I help 💯

frenzzy · 2018-07-09T07:13:13Z

render.d.ts

@@ -0,0 +1,47 @@
+export as namespace render
+
+import { ActionsType, View } from 'hyperapp'


Does this requires hyperapp to be in dependencies or peer/dev deps are enough?

Does this requires hyperapp to be in dependencies or peer/dev deps are enough?

yep, withRender(nextApp) needs ActionsType and View types.

frenzzy · 2018-07-09T07:18:31Z

test/ts/tsconfig.json

+    "jsxFactory": "h",
+    "baseUrl": "../..",
+    "typeRoots": [
+      "node_modules/@types/node/index.d.ts"


Is this also requires @types/node to be in deps? Tests are not passing...

yep, because renderToStream(view, state, actions) returns an Readable stream object.

Let me review the tests again...

- add @types/node - hyperapp

codecov · 2018-07-09T08:08:01Z

Codecov Report

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

@@          Coverage Diff          @@
##           master     #9   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          91     91           
  Branches       18     18           
=====================================
  Hits           91     91

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 bbf4f90...1243a0b. Read the comment docs.

frenzzy · 2018-07-09T08:40:52Z

@jorgebucaran or maybe it is better to publish hyperapp typescript typings to DefinitelyTyped repo? To be able to publish typings of any hyperapp project, like this one, to DefinitelyTyped too.

Otherwise it requires to add all typings dependencies to the project and users have to install them even if they do not use typescript =/

All popular projects have typings in DefinitelyTyped by similar reasons, like @types/express @types/node and many more. The only exception is the libraries written in TypeScript itself. Thoughts?

jorgebucaran · 2018-07-09T08:44:10Z

I'd prefer to provide working types out of the box, like we do in Hyperapp.

I think it's okay to take in @joseluisq changes once you are satisfied and if we run into issues in the future, e.g., when you want to change something in @hyperapp/render, you should just not forget to mention José so he can have a look.

I used to be more hesitant about TS in the past, but I think it's worth the trouble. If we can avoid forcing TS users to install another dep to add types, is a win for us.

joseluisq · 2018-07-09T08:44:15Z

Otherwise it requires to add all typings dependencies to the project and users have to install them even if they do not use typescript =/

yep, so if you want to work with Typescript you can install typings on demand.

joseluisq · 2018-07-09T08:47:58Z

I used to be more hesitant about TS in the past, but I think it's worth the trouble. If we can avoid forcing TS users to install another dep to add types, is a win for us.

@jorgebucaran right, I guess that v2 can address this on in the future.

joseluisq · 2018-07-09T08:53:18Z

In addition, Feel free to notify me or add me as reviewer.

frenzzy · 2018-07-09T09:20:01Z

so if you want to work with Typescript you can install typings on demand.

Yes, but how to install dependencies with typings "on demand" but not always? Will moving them to peerDependencies section in package.json help?

Also would be cool to support Node.js since v6. So, can we use >=6 or similar as a version number for @types/node?

joseluisq · 2018-07-09T09:39:36Z

Yes, but how to install dependencies with typings "on demand" but not always? Will moving them to peerDependencies section in package.json help?

On demand typings only if you package is in DefinitelyTyped or another repo.

Also would be cool to support Node.js since v6. So, can we use >=6 or similar as a version number for @types/node?

You right about peerDeps, it will be better to move deps to peerDeps, so users can install typings (even node versions) that they want.
But consider to add some "info message" about peer deps if the users want to use TS.

joseluisq · 2018-07-09T10:25:23Z

I have moved typing dependencies to peerDependencies a9ce19c
I have added typings to devDependencies too 1243a0b

frenzzy · 2018-07-09T10:57:02Z

@joseluisq great job, thanks!

But consider to add some "info message" about peer deps if the users want to use TS.

FYI, at the moment npm throws a warning when a peer dependency is omitted:

npm WARN @hyperapp/render@2.0.0 requires a peer of @types/node@>=6.x but none is installed. You must install peer dependencies yourself.

Personally I think this is not cool because it sounds like a mandatory, but it is more like npm related issue: npm/npm#20646

joseluisq · 2018-07-09T11:09:39Z

Personally I think this is not cool because it sounds like a mandatory, but it is more like npm related issue: npm/npm#20646

Exactly, looks not good for render case. But I hope that in the future you consider to publish the Hyperapp typings to DefinitelyTyped (@types/node reason).

So, you keep out TS Devs of your codebase. (joke 😅)

frenzzy · 2018-07-09T13:16:04Z

package.json

  },
  "scripts": {
    "lint": "eslint benchmark src test tools",
-    "test": "jest",
+    "test": "jest --coverage --no-cache && tsc -p test/ts",


Why do we need --no-cache here?

I'm not sure. jestjs/jest#6610

joseluisq · 2018-07-09T14:52:37Z

Cheers! 🥇
Waiting for the next release!

frenzzy · 2018-07-09T17:26:57Z

I made some corrections here: #10
@joseluisq could you please review

joseluisq · 2018-07-09T18:35:55Z

Sure, reviewing...

frenzzy · 2018-07-09T21:54:24Z

render.d.ts

@@ -0,0 +1,47 @@
+export as namespace render


What the convention do we have render or hyperappRender? See okwolf/hyperapp-logger#18
Looks like we agreed to use window.hyperappRender should this namespace be the same?

Yeah, I see that you agreed at okwolf/hyperapp-logger#18 to prefer hyperappLogger instead of logger.
So we should follow the same convention, something like hyperappRender.

I sent a PR about this addressed at #11

frenzzy · 2018-07-11T10:22:56Z

Released as v2.1.0 🎉

joseluisq · 2018-07-11T10:27:58Z

Cheers! 🍻

joseluisq added 6 commits July 7, 2018 23:59

add typescript declaration file

e0c0e4e

add typing file

179e805

add typescript compiling

1d1848b

add typescript api test

6777e8d

fix test via travis

446bc19

fix stream type resolution

9c395db

jorgebucaran requested a review from frenzzy July 8, 2018 02:28

frenzzy closed this Jul 8, 2018

frenzzy reviewed Jul 9, 2018

View reviewed changes

frenzzy reopened this Jul 9, 2018

add dependencies

dbc599a

- add @types/node - hyperapp

update state and actions as optional params

abb3d86

joseluisq added 2 commits July 9, 2018 12:04

move dependencies to peerDependencies

a9ce19c

add typings to devDependencies

1243a0b

frenzzy reviewed Jul 9, 2018

View reviewed changes

frenzzy approved these changes Jul 9, 2018

View reviewed changes

frenzzy merged commit afe35a5 into kriasoft:master Jul 9, 2018

frenzzy reviewed Jul 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript support #9

Typescript support #9

joseluisq commented Jul 7, 2018

frenzzy commented Jul 8, 2018

joseluisq commented Jul 8, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018 •

edited

Loading

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

frenzzy Jul 9, 2018

joseluisq Jul 9, 2018

frenzzy Jul 9, 2018

joseluisq Jul 9, 2018

joseluisq Jul 9, 2018 •

edited

Loading

codecov bot commented Jul 9, 2018 •

edited

Loading

frenzzy commented Jul 9, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

frenzzy Jul 9, 2018

joseluisq Jul 9, 2018

joseluisq commented Jul 9, 2018

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

frenzzy Jul 9, 2018

joseluisq Jul 10, 2018

joseluisq Jul 10, 2018

frenzzy commented Jul 11, 2018

joseluisq commented Jul 11, 2018

		@@ -0,0 +1,47 @@
		export as namespace render

		import { ActionsType, View } from 'hyperapp'

Typescript support #9

Typescript support #9

Conversation

joseluisq commented Jul 7, 2018

Types of changes

Checklist:

frenzzy commented Jul 8, 2018

joseluisq commented Jul 8, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018 • edited Loading

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

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

joseluisq Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 9, 2018 • edited Loading

Codecov Report

frenzzy commented Jul 9, 2018

jorgebucaran commented Jul 9, 2018

joseluisq commented Jul 9, 2018 • edited Loading

joseluisq commented Jul 9, 2018 • edited Loading

joseluisq commented Jul 9, 2018 • edited Loading

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018 • edited Loading

joseluisq commented Jul 9, 2018

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseluisq commented Jul 9, 2018

frenzzy commented Jul 9, 2018

joseluisq commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzzy commented Jul 11, 2018

joseluisq commented Jul 11, 2018

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq Jul 9, 2018 •

edited

Loading

codecov bot commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading

joseluisq commented Jul 9, 2018 •

edited

Loading