core(runner): make LH.Audit.Context immutable #10555

brendankenny · 2020-04-07T22:15:52Z

breaking change (see below for details)
fixes #9897

tl;dr: making this change now allows us to

trust audits a little less (handy for any model of plugins)
make audit contexts postMessageable, handy for an off-main-thread/extension plugin model if we choose to go in that direction
pursue the above without having to wait for the 7.0 release, whenever that will be

The change is very small since we only have two lines of code currently modifying an audit context outside of runner.js :)

The only real breaking change is that rather than audits pushing to the context to get a top-level LHR warning, they return the warning in their audit Product. It's very possible that existing extensions use this functionality. In core, there's only one audit doing so.

Note that readonly is still type-checking/advisory level enforcement, so if an audit wants to mess with things it currently can, but that's always been the case and it's useful to leave things open without a reason to do otherwise.

breaking changes:

audits that previously pushed a string to context.LighthouseRunWarnings to get a top-level warning in the LHR will instead need to return a runWarnings property on the Product return value, set to an array with that string in it. (example change from core)
previously audits could push warnings to context.LighthouseRunWarnings even if they ended up throwing an error, but that is no longer possible since the run warnings are now part of the normal return value.

brendankenny · 2020-04-07T22:17:14Z

lighthouse-core/computed/resource-summary.js

@@ -51,7 +51,7 @@ class ResourceSummary {
      'third-party': {count: 0, size: 0},
    };
    const budget = Budget.getMatchingBudget(context.settings.budgets, mainResourceURL);
-    let firstPartyHosts = /** @type {Array<string>} */ ([]);
+    let firstPartyHosts = /** @type {Immutable<Array<string>>} */ ([]);


this will be able to be just ReadonlyArray<string> after we upgrade to tsc 3.7+

brendankenny · 2020-04-07T22:18:44Z

lighthouse-core/test/runner-test.js

@@ -670,31 +670,6 @@ describe('Runner', () => {
    });
  });

-  it('includes any LighthouseRunWarnings from errored audits in LHR', () => {


this is the sole loss of functionality (being able to push top-level warnings before the audit completely errors out), but I don't think this has ever been behavior anyone cares about or needed

maybe call out the loss of this as a breaking change in PR description so we remember to include that too in final notes?

maybe call out the loss of this as a breaking change in PR description so we remember to include that too in final notes?

done

brendankenny · 2020-04-07T22:19:44Z

types/artifacts.d.ts

@@ -509,7 +509,7 @@ declare global {
      export interface MetricComputationDataInput {
        devtoolsLog: DevtoolsLog;
        trace: Trace;
-        settings: Config.Settings;
+        settings: Immutable<Config.Settings>;


this is complicated because this settings is just as often not LH.Config.Settings as it is, but that's something to sort out on a different day :)

brendankenny · 2020-04-07T22:22:12Z

types/externs.d.ts

+  // Intermediate immutable interfaces. Prefer e.g. Immutable<Set<T>> over direct use.
+  // TODO: switch to recursive type references when using tsc ≥ 3.7
+  // see https://github.com/microsoft/TypeScript/issues/13923#issuecomment-557509399
+  interface ImmutableArray<T> extends ReadonlyArray<Immutable<T>> {}


when we upgrade tsc, this can become
type ImmutableArray<T> = ReadonlyArray<Immutable<T>>;, which means things like the built-in ReadonlyArray<string> will be transparently switchable with Immutable<Array<string>>, giving a lot better ergonomics and functionality

patrickhulce

I'm not sure I trust audits any more than I did before, but OK :)

LGTM

patrickhulce · 2020-04-07T23:05:12Z

lighthouse-core/computed/computed-artifact.js

@@ -25,6 +25,8 @@ function makeComputedArtifact(computableArtifact) {
   * @return {ReturnType<C['compute_']>}
   */
  const request = (artifacts, context) => {
+    /** @type {Map<string, ArbitraryEqualityMap>} */
+    // @ts-ignore - break immutability solely for this caching-controller function.


is there a Mutable<T> type we could put Context['computedCache'] through? It'd be nice to not lose that type safety just for the mutability part

is there a Mutable<T> type we could put Context['computedCache'] through? It'd be nice to not lose that type safety just for the mutability part

Map is a little special (as are Array and Set) because typescript has a special readonly type for it that removes all the mutating functions from instances, so Maps are ReadonlyMaps, but ReadonlyMaps aren't Maps.

Mutable<T> could do an unsafe cast for that, but we could just as easily do that here too. There's little enough going on in this line of code that ignoring seemed fine, but maybe the cast would be better?

not a big deal but even the unsafe cast of ReadonlyMap to Map would be a huge upgrade IMO so we can catch the compatibility of T :)

so we can catch the compatibility of T

very good point, sold :)

I'll wait for #10461 to be landable so I can use tsc 3.7 stuff

patrickhulce · 2020-04-07T23:07:45Z

lighthouse-core/test/runner-test.js

@@ -670,31 +670,6 @@ describe('Runner', () => {
    });
  });

-  it('includes any LighthouseRunWarnings from errored audits in LHR', () => {


maybe call out the loss of this as a breaking change in PR description so we remember to include that too in final notes?

brendankenny · 2020-04-07T23:23:06Z

I'm not sure I trust audits any more than I did before, but OK :)

Maybe it would have been more correct to say it gives them better guidance on how to act :)

brendankenny · 2020-04-09T15:19:23Z

@connorjclark what should I do about this travis failure?

legacy-javascript/run.js ran because this PR touched legacy-javascript/examine-latest-run.js, but it was only a mock change, so presumably the diff happened in a different commit/due to dependency changes.

0.04s$ git diff --exit-code
diff --git a/lighthouse-core/scripts/legacy-javascript/summary-sizes.txt b/lighthouse-core/scripts/legacy-javascript/summary-sizes.txt
index 2f33adb..9f1e83b 100644
--- a/lighthouse-core/scripts/legacy-javascript/summary-sizes.txt
+++ b/lighthouse-core/scripts/legacy-javascript/summary-sizes.txt
@@ -113,8 +113,8 @@ variants/core-js-2-only-polyfill
    3410 variants/core-js-2-only-polyfill/es6-function-name/main.bundle.min.js
 
 variants/core-js-2-preset-env-esmodules
-3567 total
-2259 variants/core-js-2-preset-env-esmodules/false/main.bundle.min.js
+4121 total
+2813 variants/core-js-2-preset-env-esmodules/false/main.bundle.min.js
 1308 variants/core-js-2-preset-env-esmodules/true/main.bundle.min.js
 
 variants/core-js-3-only-polyfill
@@ -232,13 +232,13 @@ variants/core-js-3-only-polyfill
    3410 variants/core-js-3-only-polyfill/es6-function-name/main.bundle.min.js
 
 variants/core-js-3-preset-env-esmodules
-3567 total
-2259 variants/core-js-3-preset-env-esmodules/false/main.bundle.min.js
+4121 total
+2813 variants/core-js-3-preset-env-esmodules/false/main.bundle.min.js
 1308 variants/core-js-3-preset-env-esmodules/true/main.bundle.min.js
 
 variants/only-plugin
-2793 total
-1152 variants/only-plugin/-babel-plugin-transform-spread/main.bundle.min.js
+3347 total
+1706 variants/only-plugin/-babel-plugin-transform-spread/main.bundle.min.js
  827 variants/only-plugin/-babel-plugin-transform-regenerator/main.bundle.min.js
  814 variants/only-plugin/-babel-plugin-transform-classes/main.bundle.min.js
 
The command "git diff --exit-code" exited with 1.

brendankenny · 2020-04-09T16:18:55Z

my local changes from yarn test-legacy-javascript are also different than what the travis diff shows, so I'm just going to commit the travis diff and hope the cache hasn't changed in the meantime :)

I'll gate on your approval, though @connorjclark

brendankenny requested a review from a team as a code owner April 7, 2020 22:15

brendankenny requested review from patrickhulce and removed request for a team April 7, 2020 22:15

googlebot added the cla: yes label Apr 7, 2020

devtools-bot assigned patrickhulce Apr 7, 2020

devtools-bot added the waiting4reviewer label Apr 7, 2020

brendankenny commented Apr 7, 2020

View reviewed changes

patrickhulce approved these changes Apr 7, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Apr 8, 2020

brendankenny added the 6.0 label Apr 8, 2020

core(runner): make LH.Audit.Context immutable

9e65aa1

brendankenny force-pushed the immutablecontext branch from 21e13b4 to 92b3602 Compare April 8, 2020 20:29

vercel bot deployed to Preview April 8, 2020 20:29 View deployment

brendankenny mentioned this pull request Apr 9, 2020

test-legacy-javascript improvements #10566

Closed

3 tasks

vercel bot deployed to Preview April 9, 2020 16:17 View deployment

brendankenny requested a review from connorjclark April 9, 2020 16:19

brendankenny added waiting4reviewer and removed waiting4committer labels Apr 9, 2020

brendankenny added 2 commits April 9, 2020 12:21

update Immutable to use tsc 3.7 type references

c85bcba

¯\_(ツ)_/¯

9d096f5

brendankenny force-pushed the immutablecontext branch from 0135117 to 9d096f5 Compare April 9, 2020 16:22

vercel bot deployed to Preview April 9, 2020 16:22 View deployment

brendankenny assigned connorjclark Apr 9, 2020

connorjclark approved these changes Apr 9, 2020

View reviewed changes

brendankenny removed the waiting4reviewer label Apr 9, 2020

brendankenny merged commit 3e5c664 into master Apr 9, 2020

brendankenny deleted the immutablecontext branch April 9, 2020 22:20

paulirish pushed a commit that referenced this pull request Apr 14, 2020

core(runner): make LH.Audit.Context immutable (#10555)

b8e4a40

paulirish mentioned this pull request May 12, 2020

Additional 6.0 fixes googleads/publisher-ads-lighthouse-plugin#199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(runner): make LH.Audit.Context immutable #10555

core(runner): make LH.Audit.Context immutable #10555

brendankenny commented Apr 7, 2020 •

edited

Loading

brendankenny Apr 7, 2020

brendankenny Apr 7, 2020

patrickhulce Apr 7, 2020

brendankenny Apr 8, 2020

brendankenny Apr 7, 2020

brendankenny Apr 7, 2020

patrickhulce left a comment

patrickhulce Apr 7, 2020

brendankenny Apr 8, 2020

patrickhulce Apr 8, 2020

brendankenny Apr 8, 2020

patrickhulce Apr 7, 2020

brendankenny commented Apr 7, 2020

brendankenny commented Apr 9, 2020

brendankenny commented Apr 9, 2020

core(runner): make LH.Audit.Context immutable #10555

core(runner): make LH.Audit.Context immutable #10555

Conversation

brendankenny commented Apr 7, 2020 • 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

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

brendankenny commented Apr 7, 2020

brendankenny commented Apr 9, 2020

brendankenny commented Apr 9, 2020

brendankenny commented Apr 7, 2020 •

edited

Loading