fix: Correctly detect truly circular paths in JS objects, rather than objects with multiple references

[georgecrawford]

This conversation uncovered an issue whereby multiple offline calls to track() could potentially share object references (for example, in the additional context object).

The previous containsCircularPaths() implementation wasn't really looking for circular paths, but rather multiple references to the same object. See the original PR and issue.

A use-case which is common in an offline-first application such as FT App - queueing multiple events to be stored and sent when next online - is somewhat likely to include a shared reference across the events at some point in the event tracking. This causes an o-tracking does not support circular references in the analytics data error, which discards the whole queue of events.

Describe your changes

This implementation is based on https://github.com/douglascrockford/JSON-js/blob/master/cycle.js, and draws on the utility of JSON.serialize()'s replacer parameter. It also makes use of the fact that JSON.serialize() will throw a TypeError if there's a circular reference, so a normal attempt to serialize is made first, before even looking for circular references.

Note

Note that the output of jsonReplacer could potentially be used in future to send a payload to Spoor with the circular references removed. Since this is a major change in behaviour, I've left that discussion for another time.

Issue ticket number and link

Link to Figma designs

Checklist before requesting a review

• I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
• If it is a new feature, I have added thorough tests.
• I have updated relevant docs.
• I have updated relevant env variables in Doppler.

[notlee]

This looks like a fun one, thanks for the PR! Just some test naming to flag and a little clarification to ensure my understanding. o-tracking is owned by the Data Platform team now but I'm happy to approve and release this on their behalf if they are

[georgecrawford]

@notlee Thanks for your review. I've addressed your issues, but I don't think I've helped matters by force-pushing. GitHub is confused, or I've missed a step. Could you check over my changes please? Thank you!

[rowanbeentje]

I think this would work, but mostly because of the try/catch 😁 I've got a couple of nitpicks about the approach, and as we don't have proper circular detection anyway I'm wondering how good the resulting errors would be.

I'm still in favour of sending the events with some values replaced with error strings - I think that's nicer behaviour for a tracking library than dropping events?

[rowanbeentje]

Nitpick: I think this particular line now only applies to the contained function ;)

[rowanbeentje]

Question: This function doesn't quite do this - should we change the name?

Right now it does three things:

1. Try to JSON.stringify the object. If this succeeds, the object contains no circular references, and is returned.
2. If the object failed to stringify, a safe-encoding routine is run, recording circular references.
3. If circular references were found, an error is thrown.

So really the only reason for (2) is so that (3) can report the circular reference paths. And it also means this function is not safely stringifying the json - instead it's designed to error.

I think I'd be in favour of actually safely stringifying the data! I don't think we need to go as far as using the JSON schema's support, as it seems unlikely that we'd ever have a server try to reconstruct the original data. Instead I think we could replace it with a human-readable path reference, so the tracking data isn't lost; potentially also paired with a console.error - or perhaps instead an actual thrown error in a timeout, so that global error handlers can also catch/report?

[rowanbeentje]

Thought: much like the old function, I think this isn't actually detecting only circular references, is it? It's detecting object values seen before, by putting every object as a key in a weakmap and then using references if those objects are encountered again.

Your use of json.stringify() in a try/catch is hiding this from the tests I think 😁

(That said, I think the new function is a significant improvement over the old one because it has much better skipping of built-in types including null. But I think if we're relying on a list of circular references in the error, it won't be...)