TypeScript, Husky, Prettier and ESLint setup #2

TejasQ · 2019-07-08T18:49:12Z

This PR adds a nice set of dev tooling around the Entropic JS (now TS) CLI:

TypeScript for type-safety. I polyfilled "figgy-pudding" for now, until it's in definitely typed, or until add typescript definitions zkat/figgy-pudding#13 is merged.
ESLint for a little more safety on unused things and imports.
Prettier for formatting.
Husky and lint-staged for precommit checks.

I've broken this PR down into small cherry-pickable commits so we can pick and choose what we want. In case we just want TS without having all the JS files moved over, we can just remove commit eed9c9e and everything should be fine.

🚀

zkat · 2019-07-08T18:56:19Z

Could you remove the yarn.lock and install the deps using VCPM? I'd like to keep using that one until we're ready to switch over to ds itself.

TejasQ · 2019-07-08T18:57:31Z

Just noticed I had the lockfile in there! Sorry about that. What's VCPM and how do I use it? I was just going to npm i...

zkat · 2019-07-08T18:58:08Z

VCPM is how we refer to npm over here, sorry. I guess it's an inside joke I should avoid in technical convos 😅

TejasQ · 2019-07-08T18:58:43Z

Gotcha. Done! 🎉

zkat

This is all an amazing amount of work, thank you! I'd like to get some eyeballs from someone with more experience using TypeScript, if possible, and I have a few questions/comments about the stuff that's already there, if that's ok.

👏 💃

zkat · 2019-07-08T19:02:54Z

package.json

+    "@typescript-eslint/parser": "^1.11.0",
+    "eslint": "^6.0.1",
+    "eslint-config-prettier": "^6.0.0",
+    "eslint-plugin-prettier": "^3.1.0",


Do we really need eslint if we're using prettier? I thought the point of prettier was to not have to use a linter as well.

A-ha! We stubbed our toe on this on entropic as well: prettier is only a formatter & won't lint for you. You have to bring in eslint to do the linting step.

@chrisdickinson is correct.

zkat · 2019-07-08T19:04:55Z

tsconfig.json

+  "compilerOptions": {
+    /* Basic Options */
+    // "incremental": true,                   /* Enable incremental compilation */
+    "target": "es2015",


Does this mean using require? I'd like to compile down to ESM, since we're using https://npm.im/esm to support native ESM loading.

Not necessarily. This just means we use language features from ES2015 like const, let, and all of its goodness and don't transpile to an older version of JS. The module configuration is addressed by your next comment. It uses esnext modules which I believe is equivalent to ESM.

@zkat, the target could be bumped higher if you want. Depends what features you want to support sans transpilation.

zkat · 2019-07-08T19:05:21Z

tsconfig.json

+    // "incremental": true,                   /* Enable incremental compilation */
+    "target": "es2015",
+    /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */
+    "module": "esnext",


maybe this answers my above question :)

This does answer your above question.

tsconfig.json

zkat · 2019-07-08T19:06:27Z

types/figgy-pudding.d.ts

@@ -0,0 +1,75 @@
+declare module "figgy-pudding" {


this whole module is a heroic effort of types and I can't even read most of it. Amazing!

This is @forivall's great effort. I merely forked her idea and tweaked it a bit. 😍 🔥

There's a PR up in the figgy-pudding repo, so I guess see what happens with that. In the interim, including the types for that package here is fine.

Sadly, I no longer own figgy-pudding myself (it belongs to $VCPM), and I need to lock down that particular repo, which is only there for archival purposes.

In that case, a PR to DefinitelyTyped makes sense. And as mentioned you can test the types, zkat/figgy-pudding#13 (comment)

Ive submitted lots of typedefs to definitely typed, and I could take my old pr, update it and submit it there - especially as I've improved in my typedefs writing skills, and I'm sure they'd improvements to make. I've mostly abandoned the pr to figgy-pudding, as I mostly wrote it in my personal efforts to understand the npm cli, and I never ended up using figgy-pudding myself

static-raccoon · 2019-07-08T19:08:09Z

This pull request has been mentioned on Entropic. There might be relevant details there:

https://discourse.entropic.dev/t/looking-for-help-setting-up-typescript-prettier-for-ds/194/10

zacanger · 2019-07-08T23:28:20Z

.eslintrc

@@ -0,0 +1,15 @@
+{
+    "parser": "@typescript-eslint/parser",
+    "extends": [


Missing an ending newline on these two JSON files, and their format differs from the others (spaces)

zacanger · 2019-07-08T23:29:39Z

.prettierrc

@@ -0,0 +1,5 @@
+{
+  "semi": true,


This config differs from what's in github.com/entropic-dev/entropic. Would there be value in keeping them synced up?

This is a great idea! I wasn't aware that project had prettier settings! Will update.

zacanger · 2019-07-08T23:31:09Z

src/common-opts.ts

+  then: {},
+};
+
+Object.keys(module.exports).forEach(k => {


Why the mixed module types here?

So I didn't write any of this. 😉 I just moved the js to ts. @zkat has the answers to these kinds of questions. I'd say any review on their pre-existing code though is out of the scope of this PR and can be handled in other ones.

Oh right on, good to know.

zacanger · 2019-07-08T23:33:36Z

src/yargs-modules/shell.ts

+import mkCmd from "../utils/cmd-handler";
+import { Argv } from "yargs";
+
+const Shell = (module.exports = {


More mixed module systems. I've found that this can cause issues for folks who aren't super familiar with the codebase, especially as projects grow.

Please see #2 (comment)

zacanger · 2019-07-08T23:34:24Z

src/node/extensions.ts

@@ -0,0 +1,50 @@
+import fs from "fs";


Should this be import * as fs from 'fs' for correctness?

Nope. I've configured typescript for more readable imports like this. 😄

zacanger · 2019-07-08T23:36:22Z

src/node/extensions.ts

+  Module._extensions[".jsx"] = (module: any, filename: string) => {
+    const content = fs.readFileSync(filename, "utf8");
+    if (!babel) {
+      babel = require("babel-core");


These two babel packages are unmaintained, should probably be @babel/core and @babel/plugin-transform-react-jsx

Makes sense to upgrade this, but but probably in another PR as it's not related to TS, Husky, etc.

Please see #2 (comment).

zacanger · 2019-07-08T23:36:54Z

src/node/extensions.ts

+      babel = require("babel-core");
+    }
+    if (!tsPlugin) {
+      tsPlugin = require("babel-plugin-transform-typescript");


Same here, there's a @babel/plugin-transform-typescript

Again, please see #2 (comment).

zacanger · 2019-07-08T23:38:58Z

src/node/index.ts

+export function overrideNode() {
+  require("./extensions").overrideNode();
+}
+overrideNode();


Why the extra function wrapper?

Please see #2 (comment).

src/common-opts.ts

nickytonline

First pass perusing the code base. I'll grab it locally to dig a little more.

nickytonline · 2019-07-09T02:01:40Z

.lintstagedrc

+      "prettier --write",
+      "git add"
+    ],
+    "*.(tsx|ts)": [


Minor, but this can be one glob, for JS and TS, *.{js,jsx,ts,tsx}. See https://github.com/nickytonline/www.iamdeveloper.com/blob/master/package.json#L38

@zkat, it might be out of the scope of this PR, but prettier handles markdown as well. I'd say let prettier handle as much of the formatting as possible. Thoughts?

"*.md": [ "prettier --write", "git add" ]

Same goes for rust, but I don't have any experience with rust. Do they have other packages for formatting rust code?

Rust uses https://github.com/rust-lang/rustfmt

I'd say let prettier handle as much of the formatting as possible.

I'm down

nickytonline · 2019-07-09T02:09:38Z

types/figgy-pudding.d.ts

@@ -0,0 +1,75 @@
+declare module "figgy-pudding" {


There's a PR up in the figgy-pudding repo, so I guess see what happens with that. In the interim, including the types for that package here is fine.

.gitignore

nickytonline · 2019-07-09T02:35:28Z

tsconfig.json

@@ -0,0 +1,84 @@
+{


A future suggestion if more repos for entropic use TypeScript. Consider creating a master tsconfig.json and inherit from it. For an example, see https://github.com/sindresorhus/refined-github/blob/master/tsconfig.json The less code and config to maintain, the better 😉

The configuration that Sinder has, is pretty decent. You could even use that one.

Thoughts @zkat @TejasQ?

Well, yes, of course but the project is reasonably small now so I'm not sure why we'd do that? As far as I can tell, this is the first tsconfig.json in any entropic project. Naturally, we'll extend a base config if more appear.

For sure, that's why I prefaced it with A future suggestion... 😉

While I appreciate the suggestion, I suggest we do our best to avoid getting off topic and stick to reviewing the PR according to its scope. 💞

nickytonline · 2019-07-09T03:15:55Z

package.json

@@ -6,6 +6,11 @@
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"


According to the README, babel is handling the build, but while you're doing dev, it's nice to have the type checker running (npm run type-check:watch). You'd also want the type checker to run as part of the build (npm run type-check).

Suggested change

"test": "echo \"Error: no test specified\" && exit 1"

"test": "echo \"Error: no test specified\" && exit 1",

"type-check": "tsc",

"type-check:watch": "npm run type-check -- --watch",

},

I would hold off on this until there's an actual build or start script because right now (for the scope of this PR), it's too early to add such scripts.

nickytonline · 2019-07-09T03:18:07Z

tsconfig.json

+    // "composite": true,                     /* Enable project compilation */
+    // "tsBuildInfoFile": "./",               /* Specify file to store incremental compilation information */
+    // "removeComments": true,                /* Do not emit comments to output. */
+    // "noEmit": true,                        /* Do not emit outputs. */


Since babel is handling the build, we don't need to emit the output. All we care about are the declaration files.

Suggested change

// "noEmit": true, /* Do not emit outputs. */

"noEmit": true, /* Do not emit outputs. */

Please see #2 (comment).

nickytonline · 2019-07-09T03:19:55Z

Nice work @TejasQ!. Excited to see this land. In the meantime, I have to 🛌. 👋

nickytonline · 2019-07-09T03:28:27Z

.lintstagedrc

+    "*.(jsx|js)": [
+        "eslint --fix",
+        "git add"
+    ]


Suggested change

]

],

"*.md": [

"prettier --write",

"git add"

]

Co-Authored-By: Nick Taylor <[email protected]>

TejasQ · 2019-07-09T16:37:32Z

@zkat I’ve addressed all the comments. Shall we wait for more feedback or mergy mergy?

zkat

This looks great! Thank you so much for all the setup, and thanks everyone who took the time to review the PR. I'm excited to merge this now, and I'm sure we can make further adjustments as time goes on. This is an important PR because it means we're unblocked on further development for the Node side.

Thanks again! I hope you'll continue to participate in this project. I appreciate this so much!

static-raccoon · 2019-07-09T17:18:09Z

This pull request has been mentioned on Entropic. There might be relevant details there:

https://discourse.entropic.dev/t/looking-for-help-setting-up-typescript-prettier-for-ds/194/13

Tejas Kumar added 2 commits July 8, 2019 20:56

Install devDependencies

bae5893

Add initial typescript config

41af081

Tejas Kumar added 5 commits July 8, 2019 20:56

Add polyfill for figgy-pudding

1939758

Move all JS to TS

a83221a

Update gitignore to ignore compiled TS and logs

854aca4

Add prettier config

161319c

Add lint-staged config

d69af6e

TejasQ force-pushed the typescript-prettier-setup branch from 43e2862 to d69af6e Compare July 8, 2019 18:57

Update lockfile

750bf38

zkat reviewed Jul 8, 2019

View reviewed changes

zacanger reviewed Jul 8, 2019

View reviewed changes

src/common-opts.ts Show resolved Hide resolved

nickytonline reviewed Jul 9, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

nickytonline reviewed Jul 9, 2019

View reviewed changes

TejasQ and others added 4 commits July 9, 2019 06:46

Update .gitignore

f6931fe

Co-Authored-By: Nick Taylor <[email protected]>

Add newlines to EOFs

f325890

Use prettier from Entropic (thanks @zacanger)

e3a261b

Prettify markdown and refine lint-staged

69d16d4

zkat approved these changes Jul 9, 2019

View reviewed changes

zkat merged commit 4ed8d57 into entropic-dev:latest Jul 9, 2019

TejasQ deleted the typescript-prettier-setup branch July 10, 2019 15:34

		@@ -6,6 +6,11 @@
		"scripts": {
		"test": "echo \"Error: no test specified\" && exit 1"

-    "test": "echo \"Error: no test specified\" && exit 1"
+     "test": "echo \"Error: no test specified\" && exit 1",
+     "type-check": "tsc",
+     "type-check:watch": "npm run type-check -- --watch",
+  },

	// "noEmit": true, /* Do not emit outputs. */
	"noEmit": true, /* Do not emit outputs. */

TypeScript, Husky, Prettier and ESLint setup #2

TypeScript, Husky, Prettier and ESLint setup #2

Conversation

TejasQ commented Jul 8, 2019

zkat commented Jul 8, 2019

TejasQ commented Jul 8, 2019

zkat commented Jul 8, 2019

TejasQ commented Jul 8, 2019

zkat 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

TejasQ Jul 8, 2019 • 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

TejasQ Jul 8, 2019 • 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

forivall Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

static-raccoon commented Jul 8, 2019

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

TejasQ Jul 9, 2019 • 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

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

nickytonline left a comment

Choose a reason for hiding this comment

nickytonline Jul 9, 2019 • 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

nickytonline Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasQ Jul 9, 2019 • 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

nickytonline commented Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

TejasQ commented Jul 9, 2019

zkat left a comment

Choose a reason for hiding this comment

static-raccoon commented Jul 9, 2019

TejasQ Jul 8, 2019 •

edited

Loading

TejasQ Jul 8, 2019 •

edited

Loading

forivall Jul 9, 2019 •

edited

Loading

TejasQ Jul 9, 2019 •

edited

Loading

nickytonline Jul 9, 2019 •

edited

Loading

nickytonline Jul 9, 2019 •

edited

Loading

TejasQ Jul 9, 2019 •

edited

Loading

nickytonline commented Jul 9, 2019 •

edited

Loading