Break circular ref in NetworkRequest

[deepanjanroy]

This fixes #345, at least for now.

A more future-proof solution would be to use something like this: https://www.npmjs.com/package/json-stringify-safe. Although I do wonder if we should remove the option of saving artifacts altogether.

[boopathi]

Do we want to keep it as '[Circular]', or ignore the key completely ? In that case, node's util.inspect() will do the same thing - replace circular references with [Circular]. We can use it instead of JSON.stringify, maybe ?

[brendankenny]

we'll definitely want to be able to save artifacts, for things like re-running new audits or new scoring approaches over CI data as far back as the artifacts needed for the audit were available.

We will need to be more careful with serializing some of our artifacts. For NetworkRequests, for instance, we should maybe only save the publicly-exposed API and not the entire object (dealing with versioning of artifacts that come from a third-party tool like NetworkRequests is yet another problem to deal with there, though :)

[brendankenny]

json-stringify-safe looks nice, has no dependencies, and is well used, but looking at the code, it looks like a performance landmine for serializing any deep objects (moll/json-stringify-safe#17 will help quite a bit, though).

util.inspect could be OK, but is the output guaranteed to be deserializable by JSON.parse? We'd also want to check perf. It's meant more for logging than for this.

I also just remembered the critical network chains are already not serializable directly due to them containing circular references, so attempting to fully solve this with a simple layer over stringify/parse may be a lost cause anyways

[deepanjanroy]

It seems util.inspect's output is not json parsable, I'm guessing because [Circular] is not valid JSON while '[Circular]' is. I'm ok with ignoring the key completely.

Eventually, when we start working on the use case of using past gather data to run audits, we should write serializers and deserializers for all artifacts that comes from third party or has circular referencing issues.

[brendankenny]

It seems util.inspect's output is not json parsable, I'm guessing because [Circular] is not valid JSON while '[Circular]' is.

good catch

Eventually, when we start working on the use case of using past gather data to run audits, we should write serializers and deserializers for all artifacts that comes from third party or has circular referencing issues.

yeah, this is where I was starting to get to as well.

We could ease the transition by just JSON.stringifying artifacts still, and letting them handle toJSON as they see fit. Simple artifacts won't need to do anything. Deserializing is a bit harder...we could do something like have a JSON.parse reviver find optional fromJSON on gatherers using the top-level key in the serialized artifacts object.

For this PR: the serialized artifacts are currently useless except for debugging, so this change seems fine to unbreak that. Do you want to add a saveArtifacts: true (and delete artifacts.log afterwards) to the module test here to make sure we don't regress again?

[deepanjanroy]

I added it as a separate test because it felt wrong to always enable saveArtifacts in the other test, but I wish it didn't take 10 seconds.

[brendankenny]

yeah, I really wanted to avoid adding another 10 second test. The existing test is for running with a url and options, which this would still be. It isn't a unit test anyway, really a smoke test, so I don't have any problem with it being altered to exercise more lines of code than before.

[deepanjanroy]

Ah I feel less bad if I think of it as a smoke test.

[brendankenny]

LGTM!