Update handling of UA/environment in ObjectGraph instances #21
Conversation
UA: - Lock in on capture to ensure that it is an own property Environment: - Copy from (default) UA during lazy data initialization - Can be overridden (since UA string does not always capture it precisely) - Store as JSON key (because it's not always derived from UA string anymore)
|
@foolip PTAL. Once this change is in place, a form for controlling |
…d expose it as a property
src/ObjectGraph.es6.js
Outdated
| @@ -54,6 +54,14 @@ ObjectGraph.prototype.init = function(opts) { | |||
|
|
|||
| // Try to prevent recursion into internal structures. | |||
| this.blacklistedObjects.push(this); | |||
|
|
|||
| // Lock-in user agent by (potentially) copying it into an own property. | |||
There was a problem hiding this comment.
Why is just potentially? This unconditionally assigns this.userAgent, but is there some way it could have already been an own property? Is there still a reason to have ObjectGraph.prototype.userAgent now, or could have just this.userAgent, like for environment? That'd be easier to understand I think.
There was a problem hiding this comment.
The reason I would leave the prototype value in place is defensive: If we deserialize an ObjectGraph with no userAgent, it should assume its default value.
There was a problem hiding this comment.
As discussed on VC, since ObjectGraph instances are used both when collecting data on the browser and are also created with ObjectGraph.fromJSON in node when processing the data, there's a risk of confusion if the node UA string shows up there from the prototype. So setting userAgent only in a code path that's run when collecting data would be best. (Can't quite tell if this is such a code path.)
… set by owners of ObjectGraph instances
... fallback on old behaviour: Use filename. In combination with `mdittmer/web-apis` and `mdittmer/object-graph-js` changes, this allows data collectors to manually rewrite browser info (because some browsers do not contain precise enough info in their UA string), and for that info to propagate up into Confluence's data objects. Dependent PRs: mdittmer/web-apis#55 + mdittmer/web-apis#56 mdittmer/object-graph-js#21
|
I read my notification in the wrong order: mdittmer/web-apis#57 |
UA:
Environment: