Convert CLI to 100% typescript #702

samccone · 2016-09-23T23:57:47Z

But...

Generally (typescript) discourages contributions and makes things more difficult than they need to be

As to this we are already using a mix of closure annotations which have been a constant pain point even for the googlers working on this project.. I am trying to make having the wins we get from closure compiler as far as type checking much easier.

Why

The most value we are going to get from using TS is not going to be in the CLI but rather in the core of lighthouse with the large objects that we are passing around. If we land this I can work on slowing improving the type safety in that area so that the wins become even more apparent.

the end goal here is to make developing in lighthouse easier and not requiring someone to know the shape of the objects off of the top of their head that we pass around (which are quite complex).

Wins

Examples where this would have caught broken code / wrong code paths.

NaN in CLI report
Fix NaN% in CLI report #763
Incorrect indexing into audits with an object:
49dae67#r82855265
Incorrect use of Map (depending on types to be retained with used as the key of a map(
https://github.com/GoogleChrome/lighthouse/pull/702/files#diff-175d82b7239f837c6210ec1163cf38e3R47

Process changes

This CL will require a slight change in our release process to make sure someone compiles the CLI before npm publish.

samccone · 2016-09-24T00:32:08Z

just an example

paulirish · 2016-09-27T11:55:15Z

lighthouse-cli/README.md

@@ -0,0 +1,8 @@
+One time setup:
+
+`npm i -g [email protected]`


looks like typings also must be installed. any preference on version?

so we can actually now just use npm packages for all of the typings.. I might migrate us to that

samccone · 2016-09-28T03:47:14Z

@GoogleChrome/lighthouse thoughts?

ebidel · 2016-09-28T21:32:45Z

Unless there's strong motivation for maintainability, I'm not a huge fan of moving projects off of JS and requiring transpilation (looking at you .ts and .coffee). Generally, it discourages contributions and makes things more difficult than they need to be. It might be nice if you're using vscode, but ES6 features get you most of what TS offers.

samccone · 2016-09-28T21:40:01Z

Unless there's strong motivation for maintainability

This is the entire reason for this.

ES6 features get you most of what TS offers.

Not compile time checks.

makes things more difficult than they need to be.

Found this to not be true, if anything it is much easier

Generally, it discourages contributions and makes things more difficult than they need to be

As to this we are already using a mix of closure annotations which have been a constant pain point even for the googlers working on this project.. I am trying to make having the wins we get from closure compiler as far as type checking much easier.

googlebot · 2016-10-01T16:23:52Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

samccone · 2016-10-01T16:43:34Z

@googlebot it is ok

addyosmani · 2016-10-03T18:28:08Z

I think the shift over to using TS here is really tight. Having a proper type system, advanced autocompletion and better tooling around it are all awesome. Great work, @samccone 🌟

Dumb thought: it may be worth considering whether this change might increase the barrier to contribution for folks submitting PRs to LH. Similar to the days of projects being authored in CoffeeScript, it's one more thing they'll need to grok before they can contrib.

I'm personally more excited about the Flow static type checker these days which works without type annotations and is pretty good at inferring types, but this is prolly one of those calls for folks who work on the LH codebase the most often.

wardpeet · 2016-10-04T20:28:40Z

I'm not a fan of typescript but it's getting adopted pretty quickly so I wouldn't mind using it here.

samccone · 2016-10-07T00:17:17Z

This is now inline with master along with the recent changes we have landed on the CLI. 👍

paulirish · 2016-10-07T00:36:07Z

I think we should give this a shot. It's hard to grok the benefits of stuff like this until we get to try it ourselves, so I think we need to allow for that opportunity. So I propose that we land this, get a feel for it, and if we decide that the benefit is not worth the compile step, then we revert. All in all, that doesn't sound too bad.

brendankenny · 2016-10-07T06:41:38Z

Happy to talk about this in the morning. I know I've been somewhat negative on this, though I'm trying to keep an open mind.

I think the problem I'm having is that the range of reactions have been from "it discourages contributions" to "I'm not a fan but I wouldn't mind this", so the motivation for this PR is somewhat indistinguishable to me from one LH team member being really into clojure or elm and pushing it hard. The benefits (and overhead) to maintainability seem basically indistinguishable from using the Closure compiler, which incidentally no one cared enough about to maintain, and it's not clear what's changed on that front.

I'm happy enough to try this out. I like type checking and I like language tooling that actually works and I like @samccone :) But I don't like non-functional code and I don't like --watching processes and I don't like compile steps. I also agree with Addy that the first thing I do when I find a CoffeeScript codebase is close the current tab, regardless of what help I was looking for from a project.

So it would ease my mind a lot to define what success looks like or some kind of time table here instead of just eventually defaulting into more or less "there aren't enough problems to back it out".

brendankenny · 2016-10-07T06:45:49Z

lighthouse-cli/README.md

@@ -0,0 +1,9 @@
+One time setup:
+
+`npm i -g [email protected] typings`


any way we can do a local install?

yes i will modify so it can all be local, just trying to make it as easy as possible

This has been addressed

samccone · 2016-10-07T16:37:59Z

So it would ease my mind a lot to define what success looks like or some kind of time table here instead of just eventually defaulting into more or less "there aren't enough problems to back it out".

I defer to my previous answer about why

As to this we are already using a mix of closure annotations which have been a constant pain point even for the googlers working on this project.. I am trying to make having the wins we get from closure compiler as far as type checking much easier.

I am pretty confident around y'all liking this and it making development a lot easier,

in case this is not true and it is terrible and adds little to no value

We can just run this which will add closure style annotations and migrate us back to just vanilla JS.
https://github.com/angular/tsickle

The most value we are going to get from using TS is not going to be in the CLI but rather in the core of lighthouse with the large objects that we are passing around. If we land this I can work on slowing improving the type safety in that area so that the wins become even more apparent.

the end goal here is to make developing in lighthouse easier and not requiring someone to know the shape of the objects off of the top of their head that we pass around (which are quite complex).

As far as timelines let's give it 30 days, if you think it actually adds no value, and detracts from the project as far as contributors vs our current solution of closure compiler ect ect lets remove it

mprobst · 2016-10-13T17:13:29Z

Context: I'm working on TypeScript at Google on the Angular team. We've introduced TypeScript for Angular itself, into a substantial code base, and by now have helped several dozen Google projects onto TypeScript, using Angular 2, but also Angular 1, Polymer, and Vanilla JS.

Some data:

We regularly run "developer happiness" surveys with these folks. On a scale from 1-5, average self reported developer happiness is 4.3 (this is in particular for teams coming from Closure Compiler)
We measure compilation velocity (internal: go/tsdev-) across a range of large projects, and see transpilation turnaround in under a second even for large projects. Transpilation does not seem to be a hindrance in day to day work.
IDE support is a godsend, and works consistently just about everywhere (we track Sublime, Vim, IntelliJ, Eclipse, VS Code, Atom, Emacs)
People regularly find bugs through type annotations, even if the code base used Closure Compiler before. Particularly around generic types, TypeScript does a better job.

The main reason to move to TypeScript is the developer tooling though - correctness is nice, but you hopefully catch most of those issues in your test suite. In practice we find the biggest benefit to be go to symbol, auto-complete, refactoring, and the turnaround time to find that you mistyped that property before even running tests.

The main catch is the initial setup. However this is a one time cost, once you have the right npm steps in place, it's just npm watch (or whatever task runner you like, internally it's a bit different). Our experience is that these setups are very stable and reproducible, things don't break and work consistently for different developers.

Ping me directly if you'd like more pointers to data.

Re Flow, we did compare that approach with TypeScript and prefer TypeScript. Global type inference can have very surprising effects, in particular in large code bases (you change code in one file, compile errors abound across the code base). We also like the style of explicitly typing the interface to your code. Beyond that, developer tooling, ubiquity in editors, mind share etc seem to be much better with TS. YMMV.

samccone · 2016-10-20T15:22:23Z

A note.. this PR should ⚠️ not ⚠️ be squashed if merged. The commits are intended to be atomic.

Thanks

paulirish · 2016-10-21T16:00:40Z

lighthouse-cli/tsconfig.json

+        "module": "commonjs",
+        "target": "es6",
+        "noImplicitAny": false,
+        "sourceMap": true


can the map be datauri'd into the end of the .js file rather than a separate .map file?

paulirish · 2016-10-21T16:18:51Z

lighthouse-cli/tsconfig.json

+    "compilerOptions": {
+        "module": "commonjs",
+        "target": "es6",
+        "noImplicitAny": false,


what would it takes to flip this to true? is it worth it?

ask.ts(30,22): error TS7006: Parameter '_answer' implicitly has an 'any' type. bin.ts(111,10): error TS7006: Parameter 'argv' implicitly has an 'any' type. bin.ts(137,5): error TS7005: Variable 'config' implicitly has an 'any' type. bin.ts(157,3): error TS7018: Object literal's property 'fns' implicitly has an 'any[]' type. bin.ts(158,12): error TS7006: Parameter 'fn' implicitly has an 'any' type. bin.ts(162,37): error TS7006: Parameter 'c' implicitly has an 'any' type. bin.ts(183,24): error TS7006: Parameter 'addresses' implicitly has an 'any' type. bin.ts(191,11): error TS7006: Parameter 'results' implicitly has an 'any' type. bin.ts(192,11): error TS7006: Parameter 'results' implicitly has an 'any' type. bin.ts(211,27): error TS7006: Parameter 'err' implicitly has an 'any' type. bin.ts(217,22): error TS7006: Parameter 'err' implicitly has an 'any' type. chrome-finder.ts(35,9): error TS7005: Variable 'installations' implicitly has an 'any[]' type. chrome-finder.ts(43,14): error TS7006: Parameter 'inst' implicitly has an 'any' type. chrome-finder.ts(87,9): error TS7005: Variable 'installations' implicitly has an 'any[]' type. chrome-finder.ts(108,15): error TS7006: Parameter 'installations' implicitly has an 'any' type. chrome-finder.ts(108,30): error TS7006: Parameter 'priorities' implicitly has an 'any' type. chrome-finder.ts(112,10): error TS7006: Parameter 'inst' implicitly has an 'any' type. chrome-finder.ts(121,12): error TS7006: Parameter 'a' implicitly has an 'any' type. chrome-finder.ts(121,15): error TS7006: Parameter 'b' implicitly has an 'any' type. chrome-finder.ts(123,10): error TS7006: Parameter 'pair' implicitly has an 'any' type. chrome-launcher.ts(102,31): error TS7017: Index signature of object type implicitly has an 'any' type. chrome-launcher.ts(135,11): error TS7006: Parameter 'client' implicitly has an 'any' type. chrome-launcher.ts(148,28): error TS7006: Parameter 'err' implicitly has an 'any' type. chrome-launcher.ts(152,30): error TS7006: Parameter '_' implicitly has an 'any' type. chrome-launcher.ts(222,19): error TS7006: Parameter 'val' implicitly has an 'any' type. chrome-launcher.ts(222,24): error TS7006: Parameter 'def' implicitly has an 'any' type. chrome-launcher.ts(226,16): error TS7006: Parameter 'time' implicitly has an 'any' type. commands/list-audits.ts(21,48): error TS7006: Parameter 'i' implicitly has an 'any' type. printer.ts(157,31): error TS7017: Index signature of object type implicitly has an 'any' type. printer.ts(205,44): error TS7006: Parameter 'err' implicitly has an 'any' type.

I can do a follow up with all of these fixes

We had an intern fix up Angular's code base for any using a semi-automated process based on tslint. Reach out to Igor Minar or Alex Eagle for details. Though that's probably for your relatively few issues.

paulirish · 2016-10-21T16:20:28Z

lighthouse-cli/tsconfig.json

+        "module": "commonjs",
+        "target": "es6",
+        "noImplicitAny": false,
+        "sourceMap": true,


the map files everywhere add some noise. can the map be datauri'd into the end of the js files instead?

paulirish · 2016-10-21T16:21:41Z

lighthouse-cli/printer.ts

-  html: 'html'
+enum OutputMode { pretty, json, html };
+type Mode = 'pretty' | 'json' | 'html';
+const Modes = ['pretty', 'json', 'html'];


The duplication of the enum, type alias and array seems avoidable. Is there anything we can do for that?

We allow users to pass one of a know Mode for output so we need a Mode type to limit that.
We also provide a known list of safe string values Modes

We then need to index into our Modes list. Instead of doing that with a raw int value we use an enum for more readable code.

TypeScript enums can be used to map back to string values, as long as they aren't const enums:

enum E { A, B, C } console.log(E[E.A]); // 'A'

That's not always advisable – you get more code because you retain the strings, and this can lead to issues when using Closure Compiler in advanced mode. YMMV.

paulirish · 2016-10-21T16:27:10Z

lighthouse-cli/printer.ts

  }

-  return OUTPUT_MODE[mode];
+  return modeIndex;


this appears to pass off the index which just so happens to match our enum values, but that seems tenuous.

we have type checks on 68 for the input and output of this fn.. we also have an indexOf check to ensure it matches our enum values. so it is not really tenuous :)

You can explicitly give values to the enum values, enum E { A = 42, B = 23, ... }.

paulirish · 2016-10-21T16:53:00Z

lighthouse-cli/index.js

-    lighthouseRun(urls).catch(handleError);
-  } else {
-    // because you can't cancel a promise yet
-    const SIGINT = Symbol('SIGINT');


why no more symbol?

fixed, when I initially did this Symbol was not supported in TS (with out config) heh

Ok so even though we are now compiling to es2015 this does not mean that node supports what we are trying to do.

Older versions of node do not support passing symbols to event listeners

going back to just a const value

paulirish · 2016-10-21T17:05:25Z

lighthouse-cli/bin.ts

+  }
+};
+
+function launchChromeAndRun(addresses: Array<string>) {


can you put up a PR (building on this one) that fixes #805?

paulirish · 2016-10-21T17:07:34Z

lighthouse-cli/bin.ts

+            .catch(err => {
+              console.error(err);
+              console.error(err.stack);
+            }).then(() => process.exit(_ERROR_EXIT_CODE));


why the reordering here?

since we were always exiting with 130, and only in the case of errors logging, i refactored the code to DRY it up

paulirish · 2016-10-21T17:09:23Z

lighthouse-cli/bin.ts

@@ -74,6 +75,7 @@ const cli = yargs
    'perf': 'Use a performance-test-only configuration',
    'skip-autolaunch': 'Skip autolaunch of chrome when accessing port 9222 fails',
    'select-chrome': 'Choose chrome location to use when multiple installations are found',
+    'disable-network-throttling': 'Disable network throttling when running audits.',


wording nit: its not when running audits. i'd leave out the "when..."

paulirish · 2016-10-21T17:37:23Z

lighthouse-core/gather/drivers/driver.js

@@ -480,7 +489,7 @@ class Driver {
   */
  goOnline(options) {
    return this.sendCommand('Network.enable').then(_ => {
-      if (options.flags.mobile) {
+      if (options.flags.mobile && !this.opts['disable-network-throttling']) {


I don't think we want these options to be owned by the driver. They belong to the entire run, and we may want to change opts like throttling settings per pass.

Let's follow the pattern in #747 and pass the flags into beginEmulation and selectively choose which emulations to apply.

Bonus points to change mobile flag to disable-device-emulation while you're in here.

going to pull this commit an land it after

samccone · 2016-10-21T23:32:32Z

lighthouse-cli/printer.ts

-  pretty: 'pretty',
-  json: 'json',
-  html: 'html'
+enum OutputMode { pretty, json, html };


@paulirish this removes the layer of indirection :) .. using the nice reverse lookup enum name trick @mprobst tipped me off to :)

paulirish · 2016-10-22T01:33:30Z

lighthouse-cli/printer.ts

-        // Get audit object from inside of results.audits under name subitem.
-        // Coming soon events are not located inside of results.audits.
-        subitem = results.audits[subitem] || subitem;
+        let subscore: AuditResult;


subitem is a fine name. let's leave it there.

we are defining a new var with a new type so we need a new value since subitem can be either a string or a AuditResult.

if we simply redefine the subitem it still is a valid union type of string or AuditResult thus the rest of the code complains since props like extendedInfo do not exist on type string.

subScore doesn't make sense as a name for AuditResult objects.

you nuked if (subitem.comingSoon) return; and don't handle it. you can address your above conflict by filtering out the comingSoon guys this way, eh?

updated, refer to the last commit

as for 2. this was removed based on a prior convo we had about this going away. I have reverted that and now the code lines up with hiding those audit results on the CLI (as ToT currently does).

Thanks!

This is a seperate commit to retain the rename.

(this is a seperate commit to retain the git rename smarts).

Fixes #805

paulirish · 2016-10-24T14:29:16Z

lighthouse-cli/package.json

@@ -0,0 +1,11 @@
+{
+  "scripts": {


does it make sense to add a one-off compile to the parent module's postinstall and prepublish?

def not postinstall since it does not do what you expect, maybe to prepublish though.. but this is more related to how we do releases which is already pretty complex. I would prefer to simply make a release script than shoehorn the functionality into the lifecycle scripts of npm.

okay can you followup with @brendankenny then? He's looking at streamlining our release process.

Also, please add developer docs for this to the main readme asap.

paulirish

Alright I think this is looking good. Once the scripts question is settled, then we're good to land.

There is still some hesitation from folks that I'm sensitive to about landing this TS refactor, but I feel confident it's an overall win, gaining assuredness at the expense of a compile step. That said, if it's a consistent problem for people in ~1 month (by Thanksgiving), we can revert if need be.

I'm an optimist, though; let's try to make it work. :)

brendankenny changed the title ~~Initial typescript work for CLI.~~ [Proposal] Initial typescript work for CLI. Sep 24, 2016

samccone force-pushed the sjs/ts-cli branch from a022c3d to b5ca0f1 Compare September 24, 2016 00:26

samccone force-pushed the sjs/ts-cli branch 6 times, most recently from 7d91ae4 to c458f03 Compare September 24, 2016 20:56

samccone changed the title ~~[Proposal] Initial typescript work for CLI.~~ [Proposal] Convert CLI to 100% typescript Sep 24, 2016

paulirish reviewed Sep 27, 2016

View reviewed changes

samccone closed this Sep 28, 2016

paulirish deleted the sjs/ts-cli branch September 30, 2016 17:29

samccone restored the sjs/ts-cli branch October 1, 2016 16:22

samccone reopened this Oct 1, 2016

samccone force-pushed the sjs/ts-cli branch from 917cad7 to 02e6300 Compare October 1, 2016 16:36

samccone force-pushed the sjs/ts-cli branch from 9a1a212 to 1ebf7c1 Compare October 7, 2016 00:15

samccone force-pushed the sjs/ts-cli branch from 79f99a4 to e8633f4 Compare October 7, 2016 00:34

brendankenny reviewed Oct 7, 2016

View reviewed changes

samccone force-pushed the sjs/ts-cli branch 5 times, most recently from f6bc48e to ba523e8 Compare October 19, 2016 22:45

samccone force-pushed the sjs/ts-cli branch from ba523e8 to 66069d2 Compare October 21, 2016 14:55

samccone mentioned this pull request Oct 21, 2016

Make it easier to require() the CLI runner code #805

Closed

paulirish requested changes Oct 21, 2016

View reviewed changes

samccone force-pushed the sjs/ts-cli branch 4 times, most recently from 5609403 to 7b7888b Compare October 21, 2016 23:31

samccone commented Oct 21, 2016

View reviewed changes

samccone force-pushed the sjs/ts-cli branch from 7b7888b to 1f2e535 Compare October 21, 2016 23:34

paulirish reviewed Oct 22, 2016

View reviewed changes

samccone force-pushed the sjs/ts-cli branch from 1f2e535 to e0f1168 Compare October 22, 2016 18:12

samccone and others added 6 commits October 22, 2016 15:13

Migrate cli js to TS

b712120

Migrate bin.ts to typescript and add back index.js

1317340

This is a seperate commit to retain the rename.

Update chrome finder to typescript compat.

16f39cc

(this is a seperate commit to retain the git rename smarts).

Add additional typing infor to CLI to avoid NaN.

63ca3a7

Set subitem type to match the mixed type.

55a7f86

Expose bin as an easily usable component.

9a37f46

Fixes #805

samccone force-pushed the sjs/ts-cli branch from 73f684a to 9a37f46 Compare October 22, 2016 22:19

paulirish reviewed Oct 24, 2016

View reviewed changes

paulirish approved these changes Oct 24, 2016

View reviewed changes

paulirish merged commit efd7105 into master Oct 24, 2016

paulirish changed the title ~~[Proposal] Convert CLI to 100% typescript~~ Convert CLI to 100% typescript Oct 24, 2016

samccone deleted the sjs/ts-cli branch October 24, 2016 14:42

		@@ -0,0 +1,9 @@
		One time setup:

		`npm i -g [email protected] typings`

Convert CLI to 100% typescript #702

Convert CLI to 100% typescript #702

Conversation

samccone commented Sep 23, 2016 • edited Loading

But...

Why

Wins

Process changes

samccone commented Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samccone commented Sep 28, 2016

ebidel commented Sep 28, 2016

samccone commented Sep 28, 2016 • edited Loading

googlebot commented Oct 1, 2016

samccone commented Oct 1, 2016

addyosmani commented Oct 3, 2016 • edited Loading

wardpeet commented Oct 4, 2016

samccone commented Oct 7, 2016

paulirish commented Oct 7, 2016 via email

brendankenny commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samccone commented Oct 7, 2016 • edited Loading

mprobst commented Oct 13, 2016 • edited Loading

samccone commented Oct 20, 2016

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

samccone Oct 21, 2016 • 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

samccone Oct 22, 2016 • 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

paulirish left a comment

Choose a reason for hiding this comment

samccone commented Sep 23, 2016 •

edited

Loading

samccone commented Sep 24, 2016 •

edited

Loading

samccone commented Sep 28, 2016 •

edited

Loading

addyosmani commented Oct 3, 2016 •

edited

Loading

samccone commented Oct 7, 2016 •

edited

Loading

mprobst commented Oct 13, 2016 •

edited

Loading

samccone Oct 21, 2016 •

edited

Loading

samccone Oct 22, 2016 •

edited

Loading