Chore/179551806/convert to ts #11

ss-eggs · 2021-10-08T18:17:08Z

Background

Completed efforts as part of Typescriptify story:
https://www.pivotaltracker.com/n/projects/2533706/stories/179551806

Changes

installed typescript and setup config files
created fake module declarations for unavailable packages (extra lookup was needed on node-clock package)
typescriptify all available functions
updated types based on failing tests

risharma · 2021-10-13T18:38:58Z

@serhalp or @shermam when you've a moment to review this ^

serhalp · 2021-10-20T15:30:58Z

@ss-eggs could you please take a look at my review of goodeggs/goodeggs-validators#7 and start by making those same changes here? It looks like one of these PRs was copied from the other (instead of the guide: https://goodeggs.atlassian.net/wiki/spaces/ENG/pages/161841475/Porting+from+Flow+to+TypeScript), so there are a lot of the same issues. Thank you!

serhalp

Please see review on goodeggs/goodeggs-validators#7.

…typing

serhalp

Partial review: I reviewed everything except the actual source and tests! I'll review the rest a bit later.

serhalp · 2021-10-28T19:43:06Z

.npmignore

@@ -2,3 +2,4 @@
 .gitignore
 .travis.yml
 test/
+lib/


If we do this, we'll publish an unusable package that doesn't include the built code, which is the only useful thing to include!

serhalp · 2021-10-28T19:43:58Z

package.json

+    "@goodeggs/tsconfig": "^1.0.0",
+    "@types/accounting": "^0.4.2",
+    "@types/jest": "^27.0.2",
+    "@types/node": "^16.10.2",


You're testing on node 14 so maybe use 14 here?

serhalp · 2021-10-28T19:44:23Z

package.json

+    "@types/node": "^16.10.2",
+    "@types/sinon": "^10.0.4",
+    "@types/underscore": "^1.11.3",
+    "babel-jest": "^27.2.4",
    "chai": "^1.10.0",


remove? you switched to jest, so should be unused now

serhalp · 2021-10-28T19:44:30Z

package.json

    "chai": "^1.10.0",
+    "jest": "^27.2.4",
    "mocha": "~1.x.x",


same here, remove?

serhalp · 2021-10-28T19:45:29Z

package.json

+    "lint:fix": "yarn run lint:fix:es",
+    "lint:fix:es": "getk run fix-es",
+    "prepublishOnly": "yarn lint && yarn test && yarn run build",
+    "test": "jest src/",


This isn't running linting or typechecking in CI.

Suggested change

"test": "jest src/",

"test": "yarn run lint && yarn run typecheck && yarn run test:unit",

"test:unit": "jest",

serhalp · 2021-10-28T19:50:59Z

types/format-location.d.ts

@@ -0,0 +1,4 @@
+declare module 'format-location' {
+  function formatLocation(location: any, type: string): string;


Consider taking 5 mins to write a correct type here. If it's too complicated, it's ok to leave this as is.

serhalp · 2021-10-28T19:51:41Z

types/inflect.d.ts

+declare module 'inflect' {
+  export function ordinalize(arg: string): string;
+}


Could you check if we actually need this? Maybe we can remove the function that uses this? I think it's worth taking at least 10 mins to check how we're using this (e.g. in garbanzo).

Library was not created by me. I don't know if methods are being used or not in any of the apps. But I can do a search.. ticket does not specify to look if library or parts of it are being used

humanDate (not used): https://github.com/search?q=org%3Agoodeggs+humanDate&type=code
humanDay (used): https://github.com/search?q=org%3Agoodeggs+humanDay&type=code
humanShortDay (used): https://github.com/search?q=org%3Agoodeggs+humanShortDay&type=code

Let me know what you want to do.

serhalp · 2021-10-28T19:52:09Z

yarn-error.log

@@ -0,0 +1,3183 @@
+Arguments: 


Please delete this file and add yarn-error.log to .gitignore

serhalp · 2021-10-28T19:54:41Z

types/node-clock.d.ts

@@ -0,0 +1,15 @@
+declare module 'node-clock' {


😬

Since node-clock is deprecated, I would be eternally grateful if you could just refactor it away instead of adding this here. You can find hundreds of examples in this PR: https://github.com/goodeggs/garbanzo/pull/7945.

@serhalp the whole package is dependent on this node-clock package functionality.
all formatting filters https://github.com/goodeggs/goodeggs-formatters/blob/chore/179551806/convert-to-TS/src/index.ts#L148 would have to be redone in order to use moment-timeline package to provide same exposed features.
Before I continue the work, I want a sign off from you and @risharma as this was not mentioned in the ticket.

serhalp · 2021-10-28T19:55:20Z

tsconfig.declarations.json

+  "extends": "./tsconfig.json",
+  "include": [
+    "src/**/*.ts",
+    "types"


I think the correct way to do this is to remove this from here and add this instead:

"typeRoots": [ "./node_modules/@types", "./types" ],

ss-eggs · 2021-10-29T13:59:59Z

@serhalp thanks for giving a look. Ticket does not mention refactoring, so I tried not to touch code unless it is impossible add types to certain parts of code.
https://www.pivotaltracker.com/n/projects/2533706/stories/179551806

If refactoring is included - I would refactor the whole library. For now, I will complete whatever was pointed out. In my personal opinion the whole library was written poorly. In the end, I am trying to follow what is mentioned in the ticket.

…eggs-formatters in garbanzo

Fix the `formats a date range of different dates` and `formats a date range of the same day` tests

ss-eggs added 2 commits October 7, 2021 14:44

chore: converting to ts

ad62097

chore: modified tsconfig

5ccfc5a

ss-eggs requested review from adam-kramer, jer-virgo and shermam October 8, 2021 18:19

risharma requested a review from serhalp October 8, 2021 18:34

chore: added npmrc

0dbb879

chore: travis node14 yml update

bf31a32

ss-eggs force-pushed the chore/179551806/convert-to-TS branch from 39ba59c to bf31a32 Compare October 13, 2021 18:42

ss-eggs added 2 commits October 13, 2021 13:56

chore: added missing declaration setting

0e85fab

chore: goodeggs tsconfig update

14ce022

serhalp suggested changes Oct 20, 2021

View reviewed changes

chore: added tsfy changes. updated goodeggs ts config and formatters …

7ee7281

…typing

serhalp suggested changes Oct 28, 2021

View reviewed changes

missaelpadilla07 added 3 commits November 10, 2021 14:20

chore: import moment and lodash libraries

cd19364

chore: match the module for goodeggs-formatter for location

df3b753

chore: match the formats on goodeggs-formatter as the module for good…

cf550bb

…eggs-formatters in garbanzo

missaelpadilla07 self-assigned this Nov 10, 2021

missaelpadilla07 added 6 commits November 12, 2021 11:59

chore: remove clock and implement moment

9230399

chore: modify test file to avoid travis issues

6724119

update yarn.lock file to avoid the travis issues

0ace726

Fix travis issues in .formatDateRange tests suite

4f7200f

Fix the `formats a date range of different dates` and `formats a date range of the same day` tests

fix .formatDate test suite

13f5d2c

fix travis issues on shortTime and orderCutoffDateTime test suites

7557d31

missaelpadilla07 requested a review from serhalp November 12, 2021 22:25

serhalp removed request for serhalp and shermam March 14, 2022 16:43

serhalp removed request for jer-virgo and adam-kramer March 14, 2022 16:43

serhalp unassigned missaelpadilla07 Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/179551806/convert to ts #11

Chore/179551806/convert to ts #11

ss-eggs commented Oct 8, 2021

risharma commented Oct 13, 2021

serhalp commented Oct 20, 2021

serhalp left a comment

serhalp left a comment

serhalp Oct 28, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

ss-eggs Oct 29, 2021

ss-eggs Oct 29, 2021

serhalp Oct 28, 2021

serhalp Oct 28, 2021

ss-eggs Oct 29, 2021

serhalp Oct 28, 2021

ss-eggs commented Oct 29, 2021 •

edited

Loading

	"test": "jest src/",
	"test": "yarn run lint && yarn run typecheck && yarn run test:unit",
	"test:unit": "jest",

		@@ -0,0 +1,4 @@
		declare module 'format-location' {
		function formatLocation(location: any, type: string): string;

Chore/179551806/convert to ts #11

Are you sure you want to change the base?

Chore/179551806/convert to ts #11

Conversation

ss-eggs commented Oct 8, 2021

Background

Changes

risharma commented Oct 13, 2021

serhalp commented Oct 20, 2021

serhalp left a comment

Choose a reason for hiding this comment

serhalp left a 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

ss-eggs commented Oct 29, 2021 • edited Loading

ss-eggs commented Oct 29, 2021 •

edited

Loading