-
Notifications
You must be signed in to change notification settings - Fork 40
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
chore: "Soft Nav" POC #626
Conversation
Codecov Report
@@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 86.74% 86.45% -0.30%
==========================================
Files 136 137 +1
Lines 5273 5278 +5
Branches 724 734 +10
==========================================
- Hits 4574 4563 -11
- Misses 636 645 +9
- Partials 63 70 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Asset Size Report
Merging this pull request will result in the following asset size changes:
|
toArray (type, obj) { | ||
if (typeof obj !== 'object') return [] | ||
|
||
return mapOwn(obj, (key, value) => this.getValue(key, value, type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key
parameter is never used and I would like to continue removing uses of mapOwn
return mapOwn(obj, (key, value) => this.getValue(key, value, type)) | |
return Object.values(obj).map(value => this.getValue(value, type)) |
return mapOwn(obj, (key, value) => this.getValue(key, value, type)) | ||
} | ||
|
||
getValue (key, value, type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValue (key, value, type) { | |
getValue (value, type) { |
|
||
windowAddEventListener('hashchange', trackURLChange, true, this.removeOnAbort?.signal) | ||
windowAddEventListener('load', trackURLChange, true, this.removeOnAbort?.signal) | ||
windowAddEventListener('popstate', trackURLChange, true, this.removeOnAbort?.signal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both the history wrappers on lined 25-26 and this event listener?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are ports over from the original spa feature, needs eval as to what is actually needed there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So while this is less problem prone than the existing spa, it could use a big sweeping.
Overall, a lack of jsdoc, documentation behind the process, and unclear semantics make this hard to trace through. For example, on one line, I'm reading about "interactions" -> "ixn" to have it suddenly use a function that's within the BelNode
class -- is an interaction simply a bel-serializer node or is it made up of nodes? It's confusing and unclear.
The way things are passed around is also reminiscent of a top issue with writing/reading/debugging javascript code with dynamic types: I'm getting lost on what a function should pass back and both the type and available properties of objects.
The other part of the confusion-complication is the way things are structured. For example, the belnode is responsible for cancel()
-ing itself. Something that should be a simple data structure is sharing the logic/responsibility of its handler.
const shouldHold = this.checkInteractions(value) // add an interaction ID if exists | ||
if (shouldHold) return | ||
return value | ||
} | ||
|
||
checkInteractions (value) { | ||
const spaFeature = getFeatureState({ agentIdentifier: this.sharedContext.agentIdentifier, featureName: FEATURE_NAMES.basicSpa }) | ||
const { shouldHold, interactions } = spaFeature?.hasInteraction?.({ timestamp: value?.metrics?.time?.t }) || {} | ||
if (shouldHold) return true | ||
let browserInteractionId = interactions?.[0]?.id.replace('\'', '') | ||
if (browserInteractionId) value.params.browserInteractionId = browserInteractionId | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A styling remark but there's something--or a combination of things--that makes this not obvious as to what's happening.
#setLtTimer () { | ||
clearTimeout(this.#ltTimer) | ||
this.#ltTimer = setTimeout(() => { | ||
this.#resolver({ value: Math.round(Math.max(this.#startTimestamp, this.#latestLT)), attrs: { startTimestamp: this.#startTimestamp, longTasks: this.#longTasks } }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is #startTimestamp
(passed in as) and why is it some external value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--Actually see related notes in initial page load class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essentially to accommodate for either being started on the fly, or representing a past time, ie from the page load origin time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put comments in here about how this is being calculated, e.g. citing whatever source?
Also, assuming this is following webdev, it's missing both the max 2 in-flight network GET factor and the FCP should be gotten within this module so that this is a self-contained metric.
@@ -720,6 +720,8 @@ export class Aggregate extends AggregateBase { | |||
} | |||
baseEE.emit('interactionSaved', [interaction]) | |||
state.interactionsToHarvest.push(interaction) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to delete
@@ -109,6 +109,7 @@ export class Aggregate extends AggregateBase { | |||
activateFeatures(JSON.parse(responseText), this.agentIdentifier) | |||
this.drain() | |||
} catch (err) { | |||
console.error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to delete
@@ -32,6 +32,8 @@ export function lazyFeatureLoader (featureName, featurePart) { | |||
return import(/* webpackChunkName: "session_trace-aggregate" */ '../session_trace/aggregate') | |||
case FEATURE_NAMES.spa: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise can take old spa out
} | ||
|
||
hasInteraction ({ timestamp }) { | ||
if (!timestamp) return { interaction: undefined } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is meant to return interactions
? Can you align function name & property singular or plural?
} = spaFeature?.hasInteraction?.({ timestamp: event.startTime }) || {} | ||
|
||
// if the ajax happened inside an ixn window (found a match), add it to the ixn | ||
if (interactions?.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is more than 1 interaction that's returned? How can it make sense that there's more than 1 interaction at a time and that this ajax is a child of every one (multiple)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be a child of both a page load interaction and a click interaction if both are happening simultaneously while the ajax occurs under this model, since its timestamp-based alone.
Please add a one-paragraph summary here, suitable for a release notes description. This will help with documentation.
Overview
Related Issue(s)
Testing