Typescript-lite - minimal conversion to Typescript #33

rossknudsen · 2020-01-27T19:48:21Z

This is another attempt at converting to TS. This time, a minimal approach was taken since the previous attempt was too large to review. Since Babel can translate JS with Flow as well as Typescript, this works well (pretty sure the TS compiler doesn't understand Flow). So the changes are mostly to configuration files with a single example JS file converted to TS as a proof of concept (/src/index.js => /src/index.ts).

Inclusion of a .gitattributes file and changing single quotes to double quotes in the package.json scripts was introduced to help with Windows compatibility.

package.json

stephtr

To me it looks good.

connectdotz

thanks, @rossknudsen for adopting the incremental approach. I do see the potential complexity of living in flow and typescript worlds simultaneously, but I guess that is what we agree as a trade-off for the massive one-time conversion...

This incremental approach made it easier to get the typescript tooling right. I have added a few comments, let's discuss and see if we can establish a solid starting point.

connectdotz · 2020-01-29T18:57:41Z

.eslintrc.js

@@ -16,7 +16,7 @@ module.exports = {
  },
  plugins: ['flowtype', 'prettier'],
  rules: {
-    'prettier/prettier': 'error',
+    'prettier/prettier': 'off',  // disabled temporarily to prevent un-necessary delta in conversion to TS.


My vote is to keep this unchanged ('error'), as long as we keep our change progressive, I'd rather live with the whitespace diff (which we could probably turn off during diffing) than inconsistent coding style.

I didn't see any typescript specific support (@typescript-eslint/xxx). Should we add those? Not sure if it can live in the same config with "flow", worst case separate config?

My vote is to keep this unchanged ('error'), as long as we keep our change progressive, I'd rather live with the whitespace diff (which we could probably turn off during diffing) than inconsistent coding style.

May I suggest that we keep it 'off' in this MR and follow-up with a "quick" whitespace-only MR that'll be an almost automatic approval?

I didn't see any typescript specific support (@typescript-eslint/xxx). Should we add those? Not sure if it can live in the same config with "flow", worst case separate config?

👍 It's a small repo. Should we go with the recommendations? I don't know how much these diverge from our coding style. If it's bad, maybe it should follow the whitespace MR. ;). https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/TYPED_LINTING.md

Cool, happy to leave Prettier on.

Regarding @typescript-eslint/xxx linting rules/presets, I have been playing with the linting recently. Hopefully I'll get that up shortly.

connectdotz · 2020-01-29T19:01:19Z

.vscode/settings.json

-  "typescript.validate.enable": false,
-  "javascript.validate.enable": false,
+  "typescript.validate.enable": true,
+  "javascript.validate.enable": true,


with "javascript.validate.enable": true, I started to see ts error in the non-converted js files. Should we follow this for the flow js files?

So, keep it off for JS files and use the linting/flow tools from the CLI until we're through this? 👍

The Flow extension recommends turning off the JS validation because it seems to use the TS compiler which then complains about Flow type syntax being invalid. I'm happy leaving the JS validation off, but the TS validation seems to work Ok.

connectdotz · 2020-01-29T19:12:31Z

babel.config.js

@@ -5,5 +5,6 @@ module.exports = {
      { useBuiltIns: 'usage', corejs: '3' }
    ],
    '@babel/flow',
+    '@babel/typescript'


do we want to rely on babel to transpile typescript files or just have typescript (tsc) do both transpile and type checking and generation? Furthermore, I am wondering maybe we will not need babel once flow dependency is removed... therefore, maybe not add any new dependency where tsc can handle? 🤔

I don't have a strong opinion either way. Once Flow is gone, I'm AOK with just tsc.

My preference is to use the TS compiler for TS projects rather than babel.

However, while there is a mixture of JS with Flow types and TS code, I reckon that babel is a better choice. That is because babel can understand TS, Flow and JS simultaneously, while the TS compiler doesn't understand Flow (if you know of a way to precompile the Flow types out or some other voodoo let me know). I've played around converting a few other files and the difficulties soon become apparent. I might push up a couple more code changes that expose the issues.

I think in the long term, moving to a configuration that is the same/similar to what Jest itself uses would be the ideal.

connectdotz · 2020-01-29T19:15:54Z

package.json

+    "build": "babel src/ -d build/ --extensions \".js,.ts\" --source-maps inline",
+    "build:types": "tsc --emitDeclarationOnly",


if we agree to use babel for flow type only, then this should be adjusted accordingly.

See my thoughts on going with babel in the short term above.

connectdotz · 2020-01-29T19:16:50Z

package.json

@@ -18,21 +18,25 @@
    ]
  },
  "scripts": {
-    "build": "babel src/ -d build/",
+    "build": "babel src/ -d build/ --extensions \".js,.ts\" --source-maps inline",
+    "build:types": "tsc --emitDeclarationOnly",
    "prepublish": "yarn run build",


"prepublish" should call both build js and ts and types.

You OK with chaining them?

Suggested change

"prepublish": "yarn run build",

"prepublish": "yarn run build && yarn run build:types",

I noticed that there is index.d.ts in the project root. This seems to have type definitions for all the public types in the project and I didn't want to double up. Maybe when the entire codebase is TS, then include the built definitions? What do you think?

Also I can't see what steps are performed for a release - I presume the types are added manually?

Maybe when the entire codebase is TS, then include the built definitions? What do you think?

I prefer that, since we want to stick to our index.d.ts until the whole project and all types have been converted.

Also I can't see what steps are performed for a release - I presume the types are added manually?

Ah I see that the typings are referenced in the package.json, which is probably how that works.

package.json

connectdotz · 2020-01-29T19:22:35Z

tsconfig.json

+
+      /* Strict Type-Checking Options */
+      "strict": false,                          /* Enable all strict type-checking options. */
+      "noImplicitAny": false,                   /* Raise error on expressions and declarations with an implied 'any' type. */


I know why we have to have this set to true now due to many files haven't been migrated over. Please make a note that this should be reset to false once the migration is completed.

Want to make a GitHub project to track it, or should we put a TODO list on an issue? 😁

Sure, do we need an issue to track this? As mentioned above, I think we should look at making this as close to Jest's configuration as feasible. I don't think we need a comment in source, so much of the config will change once we get to 100% TS.

Cool, I added #34 to track the conversion. If you'd like to upgrade it to a project feel free.

connectdotz · 2020-01-29T20:18:21Z

tsconfig.json

+      // "isolatedModules": true,               /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */
+
+      /* Strict Type-Checking Options */
+      "strict": false,                          /* Enable all strict type-checking options. */


why not "true"?

I know right 😄

seanpoulter

Wow, thank you so much for splitting up your epic MR @rossknudsen! 🎉 It looks great. I'm sure we'll dial in the configuration as we go along. I don't think there's any of those config changes I'd consider blocking so I'll approve it for now. We can start to converge on the tsconfig from as we go.

(What were you thinking of? I had checked facebook/jest, jest-community/vscode-jest, and @orta's (?) microsoft/TypeScript-Babel-Starter).

seanpoulter · 2020-01-28T06:04:08Z

tsconfig.json

+{
+    "compilerOptions": {
+      /* Basic Options */
+      "target": "es5",                          /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */


Any reason to choose es5? Is the Node v6 in the TravisCI config the limiting factor?

I've been out of the loop for a while and forget where to check Node support for those target versions. I used to use https://node.green to check this kinda thing but it seems like that's a subset of these tables which includes https://kangax.github.io/compat-table/es6/

I think when I created the config (tsc --init) that's what it was set to. Happy to change it to your preference, do we know what runtimes that we're supporting? We might be able to bump to node v8 now.

Updated this to ES6 since we've bumped up to node v8 which the link(s) you posted seem in indicate is ES6 compatible.

seanpoulter · 2020-01-31T07:29:47Z

.eslintrc.js

@@ -16,7 +16,7 @@ module.exports = {
  },
  plugins: ['flowtype', 'prettier'],
  rules: {
-    'prettier/prettier': 'error',
+    'prettier/prettier': 'off',  // disabled temporarily to prevent un-necessary delta in conversion to TS.


My vote is to keep this unchanged ('error'), as long as we keep our change progressive, I'd rather live with the whitespace diff (which we could probably turn off during diffing) than inconsistent coding style.

May I suggest that we keep it 'off' in this MR and follow-up with a "quick" whitespace-only MR that'll be an almost automatic approval?

I didn't see any typescript specific support (@typescript-eslint/xxx). Should we add those? Not sure if it can live in the same config with "flow", worst case separate config?

👍 It's a small repo. Should we go with the recommendations? I don't know how much these diverge from our coding style. If it's bad, maybe it should follow the whitespace MR. ;). https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/TYPED_LINTING.md

seanpoulter · 2020-01-31T07:31:28Z

.vscode/settings.json

-  "typescript.validate.enable": false,
-  "javascript.validate.enable": false,
+  "typescript.validate.enable": true,
+  "javascript.validate.enable": true,


So, keep it off for JS files and use the linting/flow tools from the CLI until we're through this? 👍

seanpoulter · 2020-01-31T07:39:03Z

babel.config.js

@@ -5,5 +5,6 @@ module.exports = {
      { useBuiltIns: 'usage', corejs: '3' }
    ],
    '@babel/flow',
+    '@babel/typescript'


I don't have a strong opinion either way. Once Flow is gone, I'm AOK with just tsc.

seanpoulter · 2020-01-31T07:40:48Z

package.json

@@ -18,21 +18,25 @@
    ]
  },
  "scripts": {
-    "build": "babel src/ -d build/",
+    "build": "babel src/ -d build/ --extensions \".js,.ts\" --source-maps inline",
+    "build:types": "tsc --emitDeclarationOnly",
    "prepublish": "yarn run build",


You OK with chaining them?

Suggested change

"prepublish": "yarn run build",

"prepublish": "yarn run build && yarn run build:types",

package.json

seanpoulter · 2020-01-31T07:47:25Z

package.json

-    "prettier-project": "yarn prettier-write '?(__mocks__|src|tests)/**/*.js'"
+    "prettier-project": "yarn prettier-write \"?(__mocks__|src|tests)/**/*.+(js|ts)\"",
+    "type-check": "tsc --noEmit",
+    "type-check:watch": "npm run type-check -- --watch"


Just a non-blocking comment:

Oh JavaScript ... yarn this (prettier-write) ... npm that. 😅

seanpoulter · 2020-01-31T07:49:32Z

tsconfig.json

+
+      /* Strict Type-Checking Options */
+      "strict": false,                          /* Enable all strict type-checking options. */
+      "noImplicitAny": false,                   /* Raise error on expressions and declarations with an implied 'any' type. */


Want to make a GitHub project to track it, or should we put a TODO list on an issue? 😁

Updated the ESLint config to use appropriate parsers for each file type. Separate linting rules for each file type.

Co-Authored-By: Sean Poulter <[email protected]>

@typescript-eslint/eslint-plugin required a newer version.

package.json

rossknudsen · 2020-01-31T08:52:59Z

tsconfig.json

+{
+    "compilerOptions": {
+      /* Basic Options */
+      "target": "es5",                          /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */


I think when I created the config (tsc --init) that's what it was set to. Happy to change it to your preference, do we know what runtimes that we're supporting? We might be able to bump to node v8 now.

rossknudsen · 2020-01-31T09:07:15Z

.travis.yml

@@ -1,6 +1,6 @@
 language: node_js
 node_js:
-  - "6"
+  - "8"


I bumped the node version because it was erroring out saying that @typescript-eslint/eslint-plugin required a later version. If this is not acceptable, I can have a look at other options.

I think node 8 is acceptable, vscode-jest also supports node >= 8. @firsttris will this node version bump ok for your repo?

rossknudsen · 2020-02-07T07:54:56Z

Hey all. Just wanted to check if there was anything missing from this PR? I'm happy with it and as far as I can tell, I've either updated the code in response to comments or provided justification for the current changes. But I'm not sure what the etiquette is and this is my first contribution to OSS that has peer review.

Not being pushy, just wanting to check that I'm not holding things up 😃

connectdotz · 2020-02-08T17:08:57Z

Just wanted to check if there was anything missing from this PR?

@rossknudsen thanks for being patient, I checked the latest change and tested with vscode-jest, it all seemed fine! 👍 I think this PR will kickoff the ts migration that we all wanted for a while 😄

I will merge it later today.

Minimal conversion to Typescript.

dd84f07

rossknudsen requested review from connectdotz, seanpoulter and stephtr January 27, 2020 19:48

stephtr reviewed Jan 28, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

Added TS type check to 'ci' script and tweaked TS config to accomodate.

6208454

stephtr approved these changes Jan 29, 2020

View reviewed changes

connectdotz reviewed Jan 29, 2020

View reviewed changes

seanpoulter approved these changes Jan 31, 2020

View reviewed changes

rossknudsen and others added 4 commits January 31, 2020 21:33

Updated linting and CI task

58e81b7

Updated the ESLint config to use appropriate parsers for each file type. Separate linting rules for each file type.

Update package.json

700bad1

Co-Authored-By: Sean Poulter <[email protected]>

Replace npm with yarn

f3d8d6d

Bumped node version for CI task

005f4ed

@typescript-eslint/eslint-plugin required a newer version.

rossknudsen commented Jan 31, 2020

View reviewed changes

rossknudsen added 2 commits February 3, 2020 20:06

Fixed flow configuration to find TS modules.

e5fbffe

Updated tsconfig target from ES5 to ES6.

3640e17

connectdotz approved these changes Feb 8, 2020

View reviewed changes

connectdotz merged commit 14a1fb5 into jest-community:master Feb 8, 2020

connectdotz mentioned this pull request Feb 8, 2020

Typescript conversion #32

Closed

rossknudsen deleted the typescript-lite branch February 9, 2020 10:09

		"build": "babel src/ -d build/ --extensions \".js,.ts\" --source-maps inline",
		"build:types": "tsc --emitDeclarationOnly",

	"prepublish": "yarn run build",
	"prepublish": "yarn run build && yarn run build:types",

Typescript-lite - minimal conversion to Typescript #33

Typescript-lite - minimal conversion to Typescript #33

Conversation

rossknudsen commented Jan 27, 2020

stephtr left a comment

Choose a reason for hiding this comment

connectdotz 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

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

seanpoulter 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

rossknudsen commented Feb 7, 2020

connectdotz commented Feb 8, 2020