cli: compile out remaining typescript; add tsc type checking via jsdocs #3747

brendankenny · 2017-11-03T22:58:39Z

~~WIP PR, though the pieces are all here.~~ Interested in feedback (I've never seriously used tsc before).

Uses the same approach as puppeteer/puppeteer#986, using the TypeScript compiler's increasingly good jsdoc type support to do type checking of regular JS files. A somewhat minimal externs.d.ts file is provided for types that are implicitly or diffusely defined (cliFlags, LHR, auditResults, etc). Only a few // @ts-ignore and explicit casts added to work around current ts bugs in control flow analysis and type checking.

Uses the same async/await code used in the original ts code, but that means it won't work in Node < 8 (hence the 2/3 Travis failures). If we don't want to move to Node 8 yet (#3742), I can move those back to promises. (moved back to promises for node 6+ compat)

patrickhulce · 2017-11-03T23:20:58Z

😢

wardpeet · 2017-11-03T23:48:07Z

Just a question why move cli back to js? Just to keep consistency between everything?

brendankenny · 2017-11-06T19:28:49Z

Just a question why move cli back to js? Just to keep consistency between everything?

Ha, yeah, probably should have addressed this.

We've been split for some time with ts in cli/ and js in core/. We're never going to move core/ over to ts, but we're also not going to get Closure to successfully do type checking over core/ either (at least on reasonable timescales...still waiting on ESNext->ESNext support, better support for node-isms, etc).

If we want a unified codebase (language, but also build/test/distribution/run steps), type checking, and to minimize rewrites for the sole purpose of making our tooling happy, continuing with our current code style but checking the types with the Typescript compiler seemed like a good path forward. With chrome-launcher spun off into its own package, the amount of ts code that needed to be converted was now quite small.

Not everyone was completely sold on it, but I was going to check out the feasibility of this. Turns out it's pretty feasible :)

(re:unifying things, this PR also removes the separate install/build/test/etc steps from cli/. All of those are now run just as core/ is)

brendankenny · 2017-11-06T22:41:12Z

This is updated to what it would look like now. Very few changes to our code, at most occasionally doing something kind of awkward to satisfy --strict, e.g. explicitly removing the return value from a function that we're going to ignore anyways. I am a typescript noob, though, so there may be better ways to do some of these things.

The typescript compiler walks all dependencies it sees from included files and checks them, even if the dependencies are excluded. Rather than update runner and gather-runner and driver etc to correctly type check today, I added explicit // @ts-nocheck comments to them so any errors are ignored. Each of these are TODOs that we can remove one-by-one in the future

brendankenny · 2017-11-06T22:45:15Z

lighthouse-cli/bin.js


 const log = require('lighthouse-logger');
+// @ts-ignore


for some reason typescript won't resolve json files as part of require() (you get a "Cannot find module '../lighthouse-core/config/perf.json'" here). Searching didn't find a better solution, and @JoelEinbinder did this in puppeteer, so I'm going with that unless anyone has a better fix :)

did some searching as well, there is no support.

brendankenny · 2017-11-06T22:48:12Z

package.json

@@ -93,6 +97,9 @@
    "yargs": "3.32.0",
    "yargs-parser": "7.0.0"
  },
+  "resolutions": {


had to add this as tsc needs one version of node's d.ts file, and chrome-launcher has a specific version while everyone else just does *. See this discussion (and links in that thread) for background

patrickhulce

This is actually pretty nice! I'm pleasantly surprised. The externs typescript ease of declaration is pretty cool mix you get with this method.

patrickhulce · 2017-11-06T23:51:40Z

lighthouse-cli/bin.js

-// accept noop modules for these, so the real dependency is optional.
-import {updateNotifier} from './shim-modules';
-import {askPermission} from './sentry-prompt';
+const updateNotifier = require('update-notifier');


intentionally not shimming anymore?

intentionally not shimming anymore?

yeah, I think(?) it only added value to users not installing from package.json and managing all dependencies themselves (since we didn't add them as peerDependencies), and I don't think we have any of those users anymore, so it seemed like a good time to remove. No big deal to add back if we want to keep though

yeah that seems fine, better to be clear about our non users than half support :)

patrickhulce · 2017-11-07T00:27:54Z

lighthouse-cli/run.js

+      launchedChrome = launchedChromeInstance;
+      flags.port = launchedChrome.port;
+      return lighthouse(url, flags, config);
+    }).then(results => {


nit: let's do })\n.then

patrickhulce · 2017-11-07T00:28:29Z

lighthouse-cli/sentry-prompt.js


 const log = require('lighthouse-logger');

 const MAXIMUM_WAIT_TIME = 20 * 1000;
-// clang-format off


🚪 🔔 the witch is dead :D

patrickhulce · 2017-11-07T00:29:18Z

lighthouse-cli/sentry-prompt.js

      return result.isErrorReportingEnabled;
    }),
    timeoutPromise,
  ]);
 }

-export async function askPermission() {
+/**
+ * @return {!Promise<boolean>}


does inquirer not have types exposed? it'd be nice to reuse them if possible

we actually do use them above in prompt(). For here do you just mean so that the return type doesn't have to be explicit? Or something else.

(unfortunately one of the few places the inquirer type checking is used is broken. inquierer.promprt() returns a promise decorated with an additional ui property (allowing prompt.ui.close()), but the type declaration just calls it a promise, which is why that // @ts-ignore line has to be up there)

Oh they don't have the proper types haha got it carry on 👍

patrickhulce · 2017-11-07T00:32:31Z

package.json

    "zone.js": "^0.7.3"
  },
  "dependencies": {
+    "@types/configstore": "^2.1.1",


how many of these types do we expose in our public facing definitions? feels like a lot could be moved to devDependencies

how many of these types do we expose in our public facing definitions?

I don't actually know how this works, so happy to move them :)

If it's a transitive dep, does the compiler not need types for those as it walks dependencies? Or does it stop at the node_modules border and just checks use of the public interface of the module? (most of the @types/* we use don't have deps themselves, but some do...e.g. inquirer pulls in typings for through and a bunch of rx modules)

my understanding is that you need to include any types you expose in your .d.ts files as dependencies so when consumers try to compile they'll be able to find them.

the only case where you need to include internal type dependencies is when you expect your consumers to also be compiling your ts themselves, not just consuming the type definitions and js. since we have no ts to be compiling, it seems like we should be ok here?

this comment is what led me to believe this

cool, I'll do that then. I can't think of anywhere in our module API where we expose or depend on external types

patrickhulce · 2017-11-07T00:33:03Z

typings/externs.d.ts

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+export as namespace LH


ahhhhhh so here's the magic :)

🎩
⬇️
🐰

wardpeet · 2017-11-07T21:11:20Z

Perhaps a really stupid question but do we still need yarn closure to do the type checking?
Also are we going to force jsdocs everywhere? Does tsc does that by default or can we enable this?

brendankenny · 2017-11-08T18:55:02Z

do we still need yarn closure to do the type checking?

In theory, no. We can remove the explicit yarn closure once yarn type is expanded to check the report generator (which is all that yarn closure checks).

I imagine we'll keep yarn compile-devtools, though, which uses Closure under the hood to make sure Lighthouse can compile against devtools.

Also are we going to force jsdocs everywhere? Does tsc does that by default or can we enable this?

We can figure this out going forward. Maybe we should?

--noImplicitAny (enabled via strict: true in tsconfig) should catch when we need to add jsdocs, I believe, but we should keep an eye out for when it appears something is being correctly inferred but isn't. We could force jsdocs via eslint, but that might be too much to add all at once without introducing typing bugs, whereas adding tsc checking file-by-file allows us to be more careful and get things correct.

There are also some weird jsdoc->tsc behaviors, e.g. there are a few cases where jsdoc types are coerced to any (which I assume wouldn't flag as an implicit any?). We should keep an eye on any of those popping up.

patrickhulce

lgtm! 💯

patrickhulce · 2017-11-17T21:33:38Z

package.json

@@ -47,10 +45,16 @@
    "deploy-viewer": "cd lighthouse-viewer && gulp deploy",
    "bundlesize": "bundlesize",
    "plots-smoke": "bash plots/test/smoke.sh",
-    "changelog": "conventional-changelog -n ./build/changelog-generator/index.js -i changelog.md -s"
+    "changelog": "conventional-changelog -n ./build/changelog-generator/index.js -i changelog.md -s",
+    "type": "tsc -p ."


🚲 🏠 I'd have a slight preference for type-check or something

I like type personally. :) but w/e

patrickhulce · 2017-11-20T23:47:02Z

package.json

@@ -47,10 +45,16 @@
    "deploy-viewer": "cd lighthouse-viewer && gulp deploy",
    "bundlesize": "bundlesize",
    "plots-smoke": "bash plots/test/smoke.sh",
-    "changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file"
+    "changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file",
+    "type": "tsc -p ."


🚲 🏠 slight preference for type-check or something

brendankenny · 2017-11-21T00:26:47Z

switched to yarn type-check. As @patrickhulce notes, for people who like typing less, plain old tsc will work equivalently anywhere in the project since the compiler will walk up to the root directory looking for a tsconfig file

paulirish

i'm uncomfortably excited. like so uncomfortable it's awkward.

paulirish · 2017-11-21T01:18:09Z

package.json

@@ -47,10 +45,16 @@
    "deploy-viewer": "cd lighthouse-viewer && gulp deploy",
    "bundlesize": "bundlesize",
    "plots-smoke": "bash plots/test/smoke.sh",
-    "changelog": "conventional-changelog -n ./build/changelog-generator/index.js -i changelog.md -s"
+    "changelog": "conventional-changelog -n ./build/changelog-generator/index.js -i changelog.md -s",
+    "type": "tsc -p ."


I like type personally. :) but w/e

paulirish · 2017-11-21T01:47:08Z

tsconfig.json

+    "target": "ES2017",
+    "allowJs": true,
+    "checkJs": true,
+    "strict": true,


// "listFiles": true,

add that so its easier to verify what was checked by TSC.

for the curious, here's what's being checked now:

we can't flip it on for all of lib/* but "lighthouse-core/gather/*.js" is currently fine.

brendankenny · 2017-11-21T01:58:06Z

When this lands, anyone with an existing checkout will likely want to delete lighthouse-cli/node_modules/, lighthouse-cli/shim-modules.js, and lighthouse-cli/types/. The last two were generated by tsc and so were previously gitignored

brendankenny requested review from patrickhulce, paulirish and vinamratasingal-zz as code owners November 3, 2017 22:58

brendankenny force-pushed the clijs branch from 1a2fcac to efea8ce Compare November 4, 2017 06:06

devtools-bot added the waiting4reviewer label Nov 6, 2017

brendankenny commented Nov 6, 2017

View reviewed changes

patrickhulce reviewed Nov 7, 2017

View reviewed changes

brendankenny force-pushed the clijs branch from 5641592 to 0fcdd44 Compare November 10, 2017 20:47

brendankenny changed the title ~~cli: compile out typescript; add tsc type checking via jsdocs~~ cli: compile out remaining typescript; add tsc type checking via jsdocs Nov 11, 2017

brendankenny added 7 commits November 15, 2017 14:47

cli: compile out TS; add TSC checking of jsdoc types

45c885a

async back to promises

856c70c

add tsc type checking to tests

57841e6

fix travis

3eef710

\n.then

531e789

more @types to devDeps

c630213

remove appveyor lighthouse-cli/node_modules caching

5ff3e61

brendankenny force-pushed the clijs branch from 0fcdd44 to 5ff3e61 Compare November 15, 2017 22:50

brendankenny mentioned this pull request Nov 15, 2017

new_audit(hreflang): document has a valid hreflang code #3815

Merged

brendankenny added 3 commits November 20, 2017 14:39

Merge branch 'master' into clijs

d2e4c26

add type for recent change

f381c6c

run type checking on appveyor

e61c1cf

patrickhulce approved these changes Nov 20, 2017

View reviewed changes

type/type-check

289c9e7

paulirish approved these changes Nov 21, 2017

View reviewed changes

add listFiles comment

448a6e7

brendankenny merged commit 407b1af into master Nov 21, 2017

brendankenny deleted the clijs branch November 21, 2017 02:10

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

paulirish removed the waiting4reviewer label Mar 6, 2018

underbyte mentioned this pull request May 2, 2018

docs(readme): remove ts command for lighthouse-cli dev #5088

Merged

cli: compile out remaining typescript; add tsc type checking via jsdocs #3747

cli: compile out remaining typescript; add tsc type checking via jsdocs #3747

Conversation

brendankenny commented Nov 3, 2017 • edited Loading

patrickhulce commented Nov 3, 2017

wardpeet commented Nov 3, 2017

brendankenny commented Nov 6, 2017 • edited Loading

brendankenny commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce 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

brendankenny Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

patrickhulce Nov 7, 2017 • 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

wardpeet commented Nov 7, 2017

brendankenny commented Nov 8, 2017 • edited Loading

patrickhulce 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

brendankenny commented Nov 21, 2017

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Nov 21, 2017

brendankenny commented Nov 3, 2017 •

edited

Loading

brendankenny commented Nov 6, 2017 •

edited

Loading

brendankenny Nov 7, 2017 •

edited

Loading

patrickhulce Nov 7, 2017 •

edited

Loading

brendankenny commented Nov 8, 2017 •

edited

Loading