Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: support saving and loading error artifacts #9397

Merged
merged 6 commits into from
Jul 20, 2019
Merged

Conversation

brendankenny
Copy link
Member

fixes #4984

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.

};
}

throw new Error('Invalid value for LHError stringification');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

/* Protocol timeout failures
* Requires an additional `icuProtocolMethod` field for translation.
* Requires an additional `protocolMethod` field for translation.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driveby, I assume changed at some point and missed

class LighthouseError extends Error {
/**
* @param {LighthouseErrorDefinition} errorDefinition
* @param {Record<string, string|boolean|undefined>=} properties
* @param {Record<string, string|undefined>=} properties
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

would be nice to have this for LR too.

@@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an occasion to use Symbol()??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a Error|LighthouseError ? curious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a Error|LighthouseError ? curious.

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

};
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a wild non-null usage in stringify appears! 😮

}

/**
* A JSON.parse reviver. If any value passed in is a serialized Error or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

assert.strictEqual(auditResult.scoreDisplayMode, 'error');
assert.ok(auditResult.errorMessage.includes(errorMessage));

rimraf.sync(resolvedPath);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be done in some sort of after?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-G/-A doesn't preserve gatherer errors
5 participants