core: support saving and loading error artifacts #9397

brendankenny · 2019-07-17T22:17:20Z

Internally [gatherer errors are] real Errors, so they get serialized as {} :( This will obviously be an issue as errors won't show up in the -A stage. Presumably there will still be errors when audits try to use those artifacts, just not the ones you'd want to tell what's going on.

It would be nice to continue to use real Errors, so we should support saving and restoring errors in asset-saver if possible.

This is done through using a JSON.stringify() replacer and a JSON.parse() reviver.

It would be possible to do the stringify part by adding a toJSON to LHError, but we still use regular Error in a bunch of places (and allow regular exceptions to bubble up as gatherer errors), so we want to serialize those as well. I don't feel good about changing the global Error prototype, though, so here we are :) The implementations are basically the same, though.

Serialization is almost always gross (luckily this is strictly limited to errors and artifacts, not errors in general), so I'm happy to adapt if folks have some cleaner ideas.

brendankenny · 2019-07-17T22:19:37Z

lighthouse-core/lib/lh-error.js

+      };
+    }
+
+    throw new Error('Invalid value for LHError stringification');


this could pass through like parseReviver, but then we wouldn't have a foolproof way to have tsc type check the constructed error substitutes, so this seemed ok

brendankenny · 2019-07-17T22:20:06Z

lighthouse-core/lib/lh-error.js

@@ -227,7 +301,7 @@ const ERRORS = {
  },

  /* Protocol timeout failures
-   * Requires an additional `icuProtocolMethod` field for translation.
+   * Requires an additional `protocolMethod` field for translation.


driveby, I assume changed at some point and missed

brendankenny · 2019-07-17T22:21:31Z

lighthouse-core/lib/lh-error.js

 class LighthouseError extends Error {
  /**
   * @param {LighthouseErrorDefinition} errorDefinition
-   * @param {Record<string, string|boolean|undefined>=} properties
+   * @param {Record<string, string|undefined>=} properties


I added boolean here in #6014 but have no memory of why, and it doesn't appear to do anything in that PR or in the current codebase, so I'll assume it was from an early edit of that PR and it slipped through at the end unnoticed :)

connorjclark · 2019-07-17T22:21:36Z

would be nice to have this for LR too.

paulirish · 2019-07-17T22:21:46Z

lighthouse-core/lib/lh-error.js

@@ -56,10 +55,17 @@ const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
 * @property {boolean} [lhrRuntimeError] True if it should appear in the top-level LHR.runtimeError property.
 */

+const LHERROR_SENTINEL = '__LighthouseErrorSentinel';


is this an occasion to use Symbol()??

ah damn it. nope. won't be the same across separate executions.

paulirish · 2019-07-17T22:25:06Z

lighthouse-core/lib/lh-error.js

+   * A JSON.stringify replacer to serialize LHErrors and (as a fallback) Errors.
+   * Returns a simplified version of the error object that can be reconstituted
+   * as a copy of the original error at parse time.
+   * @param {unknown} err


why not a Error|LighthouseError ? curious.

why not a Error|LighthouseError ? curious.

that's probably better. I was originally writing it more generalized but it wasn't helpful at all

patrickhulce · 2019-07-18T17:16:11Z

lighthouse-core/lib/lh-error.js

+      };
+    }
+
+    // We still have some errors that haven't moved to be LHErrors, so serialize them as well.


any error that happens unexpectedly will always be a non-LHError, so it's a condition that's likely to exist forever :)

Suggested change

// We still have some errors that haven't moved to be LHErrors, so serialize them as well.

// Unexpected errors won't be `LHError`, but we still want to serialize them as well.

we could leave the line in if we want to also nudge folks to look into the still expected ones that aren't LHError yet?

we could leave the line in if we want to also nudge folks to look into the still expected ones that aren't LHError yet?

no, I think you're right. We should have some nudges but probably not here :)

patrickhulce · 2019-07-18T17:18:19Z

lighthouse-core/lib/lh-error.js

+  static parseReviver(key, possibleError) {
+    if (typeof possibleError === 'object' && possibleError !== null) {
+      if (possibleError.sentinel === LHERROR_SENTINEL) {
+        // eslint-disable-next-line no-unused-vars


Suggested change

// eslint-disable-next-line no-unused-vars

// eslint-disable-next-line no-unused-vars - unpack sentinel just to avoid including it in `properties`

patrickhulce · 2019-07-18T17:24:44Z

lighthouse-core/lib/asset-saver.js

-  // save everything else
-  const restArtifactsString = JSON.stringify(restArtifacts, null, 2);
+  // save everything else, using a replacer to serialize LHErrors in the artifacts.
+  const restArtifactsString = JSON.stringify(restArtifacts, stringifyReplacer, 2);


a wild non-null usage in stringify appears! 😮

patrickhulce · 2019-07-18T17:28:06Z

lighthouse-core/lib/lh-error.js

+  }
+
+  /**
+   * A JSON.parse reviver. If any value passed in is a serialized Error or


worth linking to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Using_the_reviver_parameter
?

TIL that it visits all the leaf nodes first before visiting parent objects :)

TIL that it visits all the leaf nodes first before visiting parent objects :)

yeah, I was a little worried about performance, but it doesn't seem like a big hit, and all the "optimizations" I tried to add fast paths seemed to have no net effect (so maybe the biggest hit is just the overhead from having any replacer/reviver at all)

patrickhulce · 2019-07-18T17:29:04Z

lighthouse-core/test/runner-test.js

+      assert.strictEqual(auditResult.scoreDisplayMode, 'error');
+      assert.ok(auditResult.errorMessage.includes(errorMessage));
+
+      rimraf.sync(resolvedPath);


should this be done in some sort of after?

should this be done in some sort of after?

everything is sync before it, so it seemed ok. Are there tricky jest things to worry about or anything?

no I was just thinking if the assertions fail it won't be cleaned up, but this certainly won't be the only rough edge we have in that situation across the codebase

core: support saving and loading error artifacts

be5c3cb

brendankenny requested a review from paulirish as a code owner July 17, 2019 22:17

googlebot added the cla: yes label Jul 17, 2019

brendankenny commented Jul 17, 2019

View reviewed changes

revert some stuff

e1968bd

vercel bot deployed to staging July 17, 2019 22:23 View deployment

paulirish approved these changes Jul 17, 2019

View reviewed changes

brendankenny added 2 commits July 17, 2019 15:50

tighter types

977f8a1

appveyor debugging

3ce20d2

vercel bot deployed to staging July 17, 2019 22:53 View deployment

windows path separators

12e460f

vercel bot deployed to staging July 17, 2019 23:14 View deployment

brendankenny mentioned this pull request Jul 18, 2019

i18n strings in artifacts from -G throw errors when run through -A #9269

Closed

patrickhulce approved these changes Jul 18, 2019

View reviewed changes

patrickhulce reviewed Jul 18, 2019

View reviewed changes

feedback

73c79c2

vercel bot deployed to staging July 18, 2019 20:54 View deployment

connorjclark mentioned this pull request Jul 18, 2019

clients(lightrider): serialize errors in artifacts #9410

Merged

brendankenny merged commit 6a4304a into master Jul 20, 2019

brendankenny deleted the serializeerrors branch July 20, 2019 00:49

paulirish pushed a commit that referenced this pull request Nov 6, 2019

core: support saving and loading errors in artifacts (#9397)

5c56732

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

brendankenny mentioned this pull request Sep 16, 2020

i18n: use IcuMessage objects instead of string IDs #10630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: support saving and loading error artifacts #9397

core: support saving and loading error artifacts #9397

	// We still have some errors that haven't moved to be LHErrors, so serialize them as well.
	// Unexpected errors won't be `LHError`, but we still want to serialize them as well.

	// eslint-disable-next-line no-unused-vars
	// eslint-disable-next-line no-unused-vars - unpack sentinel just to avoid including it in `properties`

core: support saving and loading error artifacts #9397

core: support saving and loading error artifacts #9397

Conversation

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