core(tsc): refactor and add type checking to config.js #5486

brendankenny · 2018-06-13T01:20:20Z

even with #5481 out of the way, this is still kind of a doozy to review, but

With most of the rest of lighthouse type checked, this involved significantly fewer changes than a few months ago
With @patrickhulce's work to unify CLI flags and the config and adding defaults for everything, a lot of simplification was already done and more was possible

Main starting change is eliminating generateNewFilteredConfig() as kind of an externally-facing API. We have a way of expressing that in the config itself now (onlyCategories, etc), so it makes sense to not expose a function for it anymore (and the extension, etc moved on to using onlyCategories, etc a while ago).

With that gone, the constructor is really the only entry point, and control flow becomes just a simple tree (ignoring deepClone and mergeOptionsOfItems):

make a copy of input config json
(optional) extend from a base config
clean settings and set defaults
clean passes and require gatherers
require audits
filter if needed
validate internal connections between audits, gatherers, etc

This means configs are always in one of two states, a ConfigJson -- possibly missing values and needing to extend something or be filtered -- and a Config -- all defaults set, filtered and extended. As mentioned in #5481, conveniently a Config is also a ConfigJson, enforced by the type system and some tests here.

brendankenny · 2018-06-13T01:23:44Z

lighthouse-core/config/config.js


    /** @type {LH.Config['settings']} */
-    this._settings = configJSON.settings || {};
+    this.settings = settings;


no reason to hide these on getters (and they don't work with JSON.stringify since the getter is on the prototype)

brendankenny · 2018-06-13T01:27:57Z

lighthouse-core/test/config/config-test.js

@@ -623,90 +683,171 @@ describe('Config', () => {
    });
  });

-  describe('generateNewFilteredConfig', () => {
+  describe('filterConfigIfNeeded', () => {


the only changes to existing tests were

to use onlyCategories (etc) instead of Config.generateNewFilteredConfig directly

to use requireAudits/requireGatherers which now call the shorthand expansion methods instead of expandAuditShorthandAndMergeOptions directly
caused a little code churn to maintain the same test, but the intention was for test path/result to be exactly the same

paulirish

love the cleanup in config.js. so good

paulirish · 2018-06-13T18:56:57Z

lighthouse-core/config/config.js


    /** @type {LH.Config['settings']} */
-    this._settings = configJSON.settings || {};
+    this.settings = settings;
    /** @type {LH.Config['passes']} */


i think these casts arent necessary any longer?

Well, they're kind of casts, but they're not coercive, so if the lhs can't be treated as these things there will be an error. That's a nice thing to have because if future changes to config.js accidentally change the lhs type, the error will be here. If not, the class's property type will just be whatever the accidental type is, and the error will instead be deep somewhere else where the property is actually used. It's also nice for file-level documentation since these are pretty much the Config instance API.

Some of this (and some of config.d.ts) can be deleted when typescript supports @implements, though (microsoft/TypeScript#17498)

patrickhulce

a bit heavier on impl changes for a type checking PR...

but it was all bad stuff, and I'm glad it's improving 😄

patrickhulce · 2018-06-13T20:28:48Z

lighthouse-core/config/config.js

    }

-    return config;
+    const {defaultPassConfig} = constants;
+    return passes.map(pass => merge(deepClone(defaultPassConfig), pass));


~~I think I may have missed this last time, but don't we need safe deepClone here for .gatherers ala deepCloneConfigJson?~~

nevermind we only clone the defaults, right? 👍

yeah, the default is an empty gatherers array so it's fine, though I guess it could be fragile if we ever change that in the future...

patrickhulce · 2018-06-13T20:31:11Z

lighthouse-core/config/config.js

    if (!audits) {
      return null;
    }

    const newAudits = audits.map(audit => {
      if (typeof audit === 'string') {
        return {path: audit, options: {}};
-      } else if (audit && typeof audit.audit === 'function') {
+      } else if ('implementation' in audit && typeof audit.implementation.audit === 'function') {
+        return audit;


the example comments for gatherer are 💯 want to add them here too?

good idea, done

patrickhulce · 2018-06-13T20:32:24Z

lighthouse-core/config/config.js

+  static filterConfigIfNeeded(config) {
+    const settings = config.settings;
+    if (!settings.onlyCategories && !settings.onlyAudits && !settings.skipAudits) {
+      return config;


want to add a return typedef? seems like we don't actually want to return anything

seems like we don't actually want to return anything

ah, yes, good call

patrickhulce · 2018-06-13T20:33:27Z

lighthouse-core/config/config.js

-        log.warn('config', `${auditId} in 'onlyAudits' is already included by ` +
-            `${foundCategory} in 'onlyCategories'`);
+      } else {
+        if (auditIds.includes(auditId) && categoryIds.includes(foundCategory)) {


want to smoosh into else if?

patrickhulce · 2018-06-13T20:35:25Z

lighthouse-core/config/config.js

-      if (!auditDefn.implementation) {
-        const path = auditDefn.path;
+    const auditDefns = expandedAudits.map(audit => {
+      let implementation;


does this become any? or does TS infer based on the later assignments?

yeah, it's any here, but by the time it's out of the if/else blocks it's typeof Audit from the assignments

patrickhulce · 2018-06-13T20:38:32Z

lighthouse-core/config/config.js

-
-            GathererClass = require(requirePath);
+    const fullPasses = passes.map(pass => {
+      const gathererDefns = Config.expandGathererShorthand(pass.gatherers).map(gathererDefn => {


this has become quite the inner loop, WDYT about extracting a method for the actual requireing part as it was before?

WDYT about extracting a method for the actual requireing part as it was before?

ok, attempted. LMKWYT

patrickhulce · 2018-06-13T20:39:59Z

lighthouse-core/test/config/config-test.js

+      const extendedConfig = new Config(extended);
+
+      assert.equal(config.passes.length, 1, 'did not filter config');
+      assert.deepStrictEqual(config, extendedConfig, 'no mutations');


should the message be "had mutations"?

brendankenny · 2018-06-13T21:27:24Z

a bit heavier on impl changes for a type checking PR...

good point. Updated the PR name :)

brendankenny · 2018-06-15T23:04:44Z

🔥🔥 ❓ 🔥🔥

patrickhulce

LGTM! 🎉

core(tsc): add type checking to config.js

bcb06d2

brendankenny requested a review from patrickhulce as a code owner June 13, 2018 01:20

brendankenny commented Jun 13, 2018

View reviewed changes

paulirish reviewed Jun 13, 2018

View reviewed changes

patrickhulce suggested changes Jun 13, 2018

View reviewed changes

brendankenny changed the title ~~core(tsc): add type checking to config.js~~ core(tsc): refactor and add type checking to config.js Jun 13, 2018

feedback

397c11a

patrickhulce approved these changes Jun 15, 2018

View reviewed changes

paulirish merged commit 8e76467 into master Jun 15, 2018

paulirish deleted the tsconfigmore branch June 15, 2018 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): refactor and add type checking to config.js #5486

core(tsc): refactor and add type checking to config.js #5486

brendankenny commented Jun 13, 2018 •

edited

Loading

brendankenny Jun 13, 2018

brendankenny Jun 13, 2018

paulirish left a comment

paulirish Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce left a comment

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018 •

edited

Loading

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

patrickhulce Jun 13, 2018

brendankenny Jun 13, 2018

brendankenny commented Jun 13, 2018

brendankenny commented Jun 15, 2018

patrickhulce left a comment

core(tsc): refactor and add type checking to config.js #5486

core(tsc): refactor and add type checking to config.js #5486

Conversation

brendankenny commented Jun 13, 2018 • edited Loading

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

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

brendankenny Jun 13, 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

brendankenny commented Jun 13, 2018

brendankenny commented Jun 15, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Jun 13, 2018 •

edited

Loading

brendankenny Jun 13, 2018 •

edited

Loading