extension(tsc): add type checking to extension entry points #5346

brendankenny · 2018-05-24T20:20:44Z

to do this, needed to do a little import/export doctoring.

lighthouse-background.js

exports functions to window if not in node (this is how devtools uses it), otherwise exports function as a node module

lighthouse-ext-background.js

requires lighthouse-background.js in module form, allowing cross-file references to work in type checking and IDE navigation. Browserify handles the require normally (before it was still required but not as a module, but wrote its exports to window so they were available there)
exports functions to window, since this file is only ever used where there is a window, but also to a module so popup.js can access for type checking

popup.js

has a typedef import of lighthouse-ext-background.js only for types, so still works as before, but now types of calls to the background page are checked and IDE nav, etc works

git isn't picking up that lighthouse-ext-background.js moved to extension-background.js, so it's treating it as a totally new file. Sorry about that. I can revert the move and do that afterwards if that's easier to review. reverted file renaming for now

brendankenny · 2018-05-24T20:22:40Z

lighthouse-core/gather/connections/raw.js

@@ -10,22 +10,11 @@ const Connection = require('./connection.js');
 /* eslint-disable no-unused-vars */

 /**
- * @interface


turns out this was never working as an interface for tsc (@interface has closure support only). Making a typedef so can be imported elsewhere

brendankenny · 2018-05-24T20:23:22Z

lighthouse-extension/app/src/chromereload.js

@@ -1,29 +0,0 @@
-/**


I don't think anyone uses this, correct? Just came with the extension generator :)

i do sometimes but dont most of the time. happy to get rid of it.

patrickhulce · 2018-05-24T20:23:41Z

lighthouse-extension/app/manifest_canary.json

@@ -12,8 +12,7 @@
  "default_locale": "en",
  "background": {
    "scripts": [
-      "scripts/chromereload.js",


should we just delete this are we using manifest canary?

I'm good with deleting, but we should probably delete the actual extension in that case? @paulirish

ill delete canary extension now
edit: done.

brendankenny · 2018-05-24T20:35:13Z

ok, yeah, that's way better without the file renames :) will propose that in a future PR

brendankenny · 2018-05-24T20:41:01Z

lighthouse-extension/app/src/lighthouse-ext-background.js

 let lighthouseIsRunning = false;
-let latestStatusLog = [];
-
-// /**


this was disabled in February 2017. I don't think we need to leave it in anymore just in case :)

patrickhulce

seems mostly good to me! 👍

patrickhulce · 2018-05-24T20:27:20Z

lighthouse-extension/app/src/bundle-entry.js

-};
+}
+
+if (typeof module !== 'undefined' && module.exports) {


can we just comment that this is for browserify and that it's not really executed in node? this threw me a bit :)

patrickhulce · 2018-05-24T20:29:19Z

lighthouse-extension/app/src/popup.js

+ * @param {ParentNode} context
+ * @return {HTMLElement}
+ */
+function find(query, context) {


we seem to always use document, do we need context?

added default of document. WDYT?

sure that sg 👍

patrickhulce · 2018-05-24T20:36:50Z

lighthouse-core/index.js

 */
-function lighthouse(url, flags = {}, configJSON) {


this is a breaking change for lighthouse('http://example.com') in node, do we really need to do this?

do we support fully optional flags? I hadn't dug into it so I didn't pursue it ({} isn't currently castable to LH.Flags which is why I dropped it), but if so, agreed we should support that here.

I can add a manual cast of a {} here, maybe, and then follow up with solidifying the flags pipeline in a follow up?

would Partial<LH.Flags> cut it? I guess we could differentiate LH.Flags from LH.CLIFlags which is basically what's defined now

would Partial<LH.Flags> cut it? I guess we could differentiate LH.Flags from LH.CLIFlags which is basically what's defined now

yeah, I think that's basically what we need to do. Handy to have output from yargs, but then our optional flags fed into Config is a different beast (and it looks like completely optional is fine).

Ok if we do a future "Make Flags Good" PR while leaving this with the {} default?

patrickhulce · 2018-05-24T20:40:04Z

lighthouse-extension/app/src/lighthouse-ext-background.js

+  const runnerResult = await background.runLighthouseForConnection(connection, url, options,
+    categoryIDs, updateBadgeUI);
+  if (runnerResult) {
+    // For now, should always be a runnerResult as the extension can't do `gatherMode`


should we throw if no runner result? seems like a fatal condition

patrickhulce · 2018-05-24T20:42:00Z

lighthouse-extension/app/src/lighthouse-ext-background.js

  chrome.runtime.onInstalled.addListener(details => {
    if (details.previousVersion) {
      // eslint-disable-next-line no-console
      console.log('previousVersion', details.previousVersion);
    }
  });
 }
+
+if (typeof module !== 'undefined' && module.exports) {
+  // We don't want tsc to infer an index type, so use exports intest of module.exports.


who consumes this via require?

who consumes this via require?

popup.js imports this for a typedef only. Added a comment to explain

oh just for typedefs gotcha 👍 yeah comment is good

paulirish · 2018-05-24T23:28:24Z

lighthouse-extension/app/src/chromereload.js

@@ -1,29 +0,0 @@
-/**


i do sometimes but dont most of the time. happy to get rid of it.

paulirish · 2018-05-24T23:49:47Z

lighthouse-extension/app/src/lighthouse-ext-background.js

 let lighthouseIsRunning = false;
-let latestStatusLog = [];
-
-// /**


paulirish · 2018-05-24T23:50:31Z

lighthouse-extension/app/src/lighthouse-background.js

-};
+}
+
+if (typeof module !== 'undefined' && module.exports) {


really dig this. thanks.

paulirish · 2018-05-24T23:57:12Z

lighthouse-extension/app/src/lighthouse-ext-background.js

-  return window.runLighthouseForConnection(connection, url, options, categoryIDs)
-    .then(results => {
-      const endTime = Date.now();
-      results.timing = {total: endTime - startTime};


currently LR continues to expect a .timing.total prop.

currently LR continues to expect a .timing.total prop.

this is added in runner now, so doesn't need to be done here

paulirish · 2018-05-25T00:02:47Z

lighthouse-extension/app/src/popup.js

-    } else {
-      hideRunningSubpage();
+async function initPopup() {
+  chrome.tabs.query({active: true, lastFocusedWindow: true}, function(tabs) {


is changing the order here deliberate?

is changing the order here deliberate?

yeah, it used to fire off the promise to retrieve the background page and immediately run this query in parallel. Since I switched the promise.then to await, the query wouldn't run until after the background page had been found and the rest of the init stuff had run. Since at least siteURL is used for the pre-populated github message in case of a thrown error, it made sense to make sure it was set before the stuff

paulirish · 2018-05-25T00:50:25Z

👍 to a file-rename PR afterwards. :)

brendankenny · 2018-05-29T20:37:37Z

@patrickhulce second (third?) look?

brendankenny added 5 commits May 24, 2018 12:44

start type checking runners

c9b8eef

add type checking to extension background scripts

d42535a

s/lighthouse-ext-background/extension-background

fae3482

fixup of extension-background export

c3db014

s/lighthouse-background/bundle-entry

8d5df1d

brendankenny requested review from patrickhulce, paulirish and vinamratasingal-zz as code owners May 24, 2018 20:20

paulirish deployed to PR staging May 24, 2018 20:20 View deployment

brendankenny commented May 24, 2018

View reviewed changes

patrickhulce reviewed May 24, 2018

View reviewed changes

revert extension file renaming

830a6fe

brendankenny commented May 24, 2018

View reviewed changes

patrickhulce reviewed May 24, 2018

View reviewed changes

feedback

64b4802

paulirish reviewed May 25, 2018

View reviewed changes

remove manifest_canary.json

9d83fe9

paulirish approved these changes May 25, 2018

View reviewed changes

patrickhulce approved these changes May 29, 2018

View reviewed changes

patrickhulce merged commit 92728c9 into master May 29, 2018

patrickhulce deleted the tsrun branch May 29, 2018 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extension(tsc): add type checking to extension entry points #5346

extension(tsc): add type checking to extension entry points #5346

brendankenny commented May 24, 2018 •

edited

Loading

brendankenny May 24, 2018

brendankenny May 24, 2018

paulirish May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

paulirish May 24, 2018 •

edited

Loading

brendankenny commented May 24, 2018

brendankenny May 24, 2018

paulirish May 24, 2018

patrickhulce left a comment

patrickhulce May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

patrickhulce May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

patrickhulce May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

patrickhulce May 24, 2018

brendankenny May 24, 2018

patrickhulce May 24, 2018

paulirish May 24, 2018

paulirish May 24, 2018

paulirish May 24, 2018

paulirish May 24, 2018

brendankenny May 25, 2018

paulirish May 25, 2018

brendankenny May 25, 2018

paulirish commented May 25, 2018

brendankenny commented May 29, 2018

extension(tsc): add type checking to extension entry points #5346

extension(tsc): add type checking to extension entry points #5346

Conversation

brendankenny commented May 24, 2018 • 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

paulirish May 24, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny commented May 24, 2018

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

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

paulirish commented May 25, 2018

brendankenny commented May 29, 2018

brendankenny commented May 24, 2018 •

edited

Loading

paulirish May 24, 2018 •

edited

Loading