[Refactoring] convert hosting/*.js to typescript code #1903

iida-hayato · 2020-01-08T11:50:06Z

convert to typescript code
- serve/hosting.ts
- hosting/initMiddleware
- hosting/normalizedHostingConfigs
- hosting/implicitInit
check npm t
check npm run lint
local test

coveralls · 2020-01-08T11:54:22Z

Coverage increased (+0.01%) to 65.581% when pulling 7c3def0 on iida-hayato:ts-hosting into 9ca80cf on firebase:master.

samtstern · 2020-01-08T21:10:33Z

src/hosting/normalizedHostingConfigs.ts

+import * as _ from "lodash";

-function _filterOnly(configs, onlyString) {
+type HostingConfig = { site: string; target: string };


Any reason to use a type here but use an interface for TemplateServerResponse?

it's no reason.
it seems more better to interface.

please prefer interfaces to types. Maybe that's something I could enforce with the linter in the future...

samtstern · 2020-01-08T21:13:54Z

@iida-hayato thanks for doing this! I left some small comments, hopefully @bkendall can do a review as well.

iida-hayato · 2020-01-09T06:03:19Z

There seems to be a better name for the added types.
such as

TemplateServerResponce
HostingConfig
ApplicationHandler

But i have no idea . 🤔

bkendall

Alright, you prompted me to write up a script that should help identify easy bits to clean up while rewriting this code. If you would do the following, it would be appreciated:

Make sure your local master branch is up to date and sync this branch with it.
Run npm run lint:changed-files - it should print 28 "problems" (which are lint warnings) that should be addressed. Most of it will be fairly trivial, but worth addressing.

One of the tricker warnings to get rid of in some of the files will be @typescript-eslint/no-explicit-any, especially when it comes to req, res, and next. If you get stuck, silence that warning by adding a comment to the end of the line: // eslint-disable-line @typescript-eslint/no-explicit-any (if the formatter complains, you can use // eslint-disable-next-line @typescript-eslint/no-explicit-any above it instead).

src/hosting/implicitInit.ts

bkendall · 2020-01-10T18:03:29Z

src/hosting/implicitInit.ts

+/**
+ * generate template server responce
+ * @param options
+ * @return {Promise<{js: string, json: string}>}


Specifying return types in a typescript file is redundant. Please just include a brief written description of what is returned. e.g.:

Suggested change

* @return {Promise<{js: string, json: string}>}

* @return the initialization configuration.

bkendall · 2020-01-10T18:05:19Z

src/hosting/normalizedHostingConfigs.ts

+import * as _ from "lodash";

-function _filterOnly(configs, onlyString) {
+type HostingConfig = { site: string; target: string };


please prefer interfaces to types. Maybe that's something I could enforce with the linter in the future...

src/hosting/normalizedHostingConfigs.ts

bkendall · 2020-01-10T18:06:56Z

src/hosting/normalizedHostingConfigs.ts

-module.exports = function(options) {
+/**
+ * @param options
+ * @return {any[] | HostingConfig[]}


lint --fix adds this return type? I will dig into that as it shouldn't in TS...

src/serve/hosting.ts

iida-hayato · 2020-01-15T12:44:05Z

Generated JSDocs seems useless, so I erased and rewrite JSDoc comments.
Tell me if I wrote wrong comments.

bkendall · 2020-01-16T18:20:58Z

FYI - I've still got this on my radar, but dedicating the time to make sure I can go through it is a little tricky. I'm hoping to get back to you today or tomorrow. Thanks for the PR and your patience.

iida-hayato · 2020-01-17T09:45:13Z

@bkendall

All right.
today, I fixed code as I can.

If this still difficult to merge, you can close it.

or, Is there any other code that preferentially converts to typescript?
I can help that.

bkendall · 2020-01-24T22:38:33Z

Thanks for keeping on top of this. I've been horrible at actually getting back to you on this and I apologize for that - things have been rather busy for me as of late as other things have taken priority. I appreciate the PR and promise I haven't lost track of it!

iida-hayato · 2020-02-03T03:56:10Z

Because base's hosting.js has been changed,
It would be better to close this PR and recreate a new PR 🤔

bkendall · 2020-02-03T20:48:02Z

I've been super super busy, so I haven't gotten time to sit down on this, but I was going to manually resolve the conflicts and merge this. If you can manage to resolve the conflicts yourself in the next few days, I will promise to re-review by the end of the week. Otherwise, it may be next week when I get a chance to wrap this up.

bkendall · 2020-02-03T20:48:25Z

(seriously, thank you for your patience and I'm sorry I've been slooww)

bkendall · 2020-02-19T16:49:30Z

🎶 I'm horrible, it's true 🎶

(to the tune of You're Beautiful)

I'm looking at this right now!

iida-hayato added 25 commits December 19, 2019 15:38

serve/index.js to typescript

4ae28e0

convert serve/index js to typescript code

661b0a9

change require to import

6f5d918

change export serve and fix relative source

dfceecb

convert arrow function

7640ad0

function serve return type

09ccba6

add jsdoc

7a78c7f

refactoring remove const target

8e7d450

Merge branch 'master' into master

cc56cb7

Merge branch 'master' into master

9dc5468

Merge branch 'master' into master

2d4657c

Merge branch 'master' into master

034c74e

change hosting.js to hosting.js

2d02b29

fix build error

eadf13a

change require to import statements

84bcd24

remove blank line

2efe3f9

fix export

49189f9

append functions type

70c3f75

change implicitInit.js to ts

0121849

fix ts build errors

36268e1

change initMiddleware to ts

a2ba608

fix lint

8b1c2c4

change normalizedHostingConfigs to ts

2bc2c09

change functons to arrow function

fb55e67

append type in normalizedHostingConfigs

741bde8

iida-hayato changed the title ~~[Refactoring] convert hosting/*.js to typescript code~~ [WIP][Refactoring] convert hosting/*.js to typescript code Jan 8, 2020

googlebot added the cla: yes Manual indication that this has passed CLA. label Jan 8, 2020

Merge branch 'master' into ts-hosting

7c3def0

samtstern reviewed Jan 8, 2020

View reviewed changes

samtstern requested a review from bkendall January 8, 2020 21:13

iida-hayato added 2 commits January 9, 2020 15:03

Merge branch 'master' into ts-hosting

942d896

Merge branch 'master' into ts-hosting

595e60b

bkendall requested changes Jan 10, 2020

View reviewed changes

iida-hayato added 6 commits January 13, 2020 00:54

Merge branch 'master' into ts-hosting

d3ecc8b

Merge branch 'master' into ts-hosting

709c988

Merge branch 'master' into ts-hosting

c2d0cad

fix optional type to key?

79ff451

fix type responce to response

2705039

rewrite JSDocs

dc15e9c

iida-hayato added 4 commits January 15, 2020 21:45

remove unnecessary underscore

c2c098a

remove unnecessary underscore

a1dcecd

remove unnecessary underscore

c4dc1a1

add missing async

914eb42

iida-hayato added 3 commits January 17, 2020 10:35

Merge branch 'master' into ts-hosting

c726084

fix type to interface

74d6523

fix no-explicit-any

fc0a67a

iida-hayato changed the title ~~[WIP][Refactoring] convert hosting/*.js to typescript code~~ [Refactoring] convert hosting/*.js to typescript code Jan 17, 2020

bkendall mentioned this pull request Feb 19, 2020

[Refactoring] convert hosting/*.js to typescript code, part 2 #1984

Merged

bkendall closed this in #1984 Feb 19, 2020

	* @return {Promise<{js: string, json: string}>}
	* @return the initialization configuration.

[Refactoring] convert hosting/*.js to typescript code #1903

[Refactoring] convert hosting/*.js to typescript code #1903

Uh oh!

Conversation

iida-hayato commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

iida-hayato Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

bkendall Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 8, 2020

Uh oh!

iida-hayato commented Jan 9, 2020

Uh oh!

bkendall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkendall Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

bkendall Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkendall Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iida-hayato commented Jan 15, 2020

Uh oh!

bkendall commented Jan 16, 2020

Uh oh!

iida-hayato commented Jan 17, 2020

Uh oh!

bkendall commented Jan 24, 2020

Uh oh!

iida-hayato commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkendall commented Feb 3, 2020

Uh oh!

bkendall commented Feb 3, 2020

Uh oh!

bkendall commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iida-hayato commented Jan 8, 2020 •

edited

Loading

coveralls commented Jan 8, 2020 •

edited

Loading

iida-hayato commented Feb 3, 2020 •

edited

Loading