core(navigation-runner): only run getArtifact phase once
#15827
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| for (const definition of phaseState.artifactDefinitions) { | ||
| const {instance} = definition.gatherer; | ||
| if (instance instanceof DevtoolsLog) { | ||
| devtoolsLog = instance.getDebugData(); |
There was a problem hiding this comment.
but... why not just getArtifact() ? types?
There was a problem hiding this comment.
I want to separate the getArtifact logic from the getDebugInfo logic even if they happen to be the same. Right now when designing gatherers we assume that getArtifact is only run once at the end of gathering. If we were to just reuse getArtifact here, it would break that assumption.
It's really just defensive coding to prevent future regressions if the impl of dtlog/trace change.
| @@ -110,32 +110,20 @@ async function _navigate(navigationContext) { | |||
| * @return {Promise<{devtoolsLog?: LH.DevtoolsLog, records?: Array<LH.Artifacts.NetworkRequest>, trace?: LH.Trace}>} | |||
| */ | |||
| async function _collectDebugData(navigationContext, phaseState) { | |||
| const devtoolsLogArtifactDefn = phaseState.artifactDefinitions.find( | |||
There was a problem hiding this comment.
oh boy this is a pleasant diff to witness :)
| let devtoolsLog; | ||
| let trace; | ||
|
|
||
| for (const definition of phaseState.artifactDefinitions) { |
There was a problem hiding this comment.
Did this change from a .find to a loop for some reason, or just preference?
I guess it got rid of the filter and is cleaner, jw.
There was a problem hiding this comment.
It's cleaner and made the type narrowing easier.
There was a problem hiding this comment.
oh! you did this b/c need to narrow types to use getDebugData
@connorjclark noticed that we are running the
getArtifactphase of the DT log and trace twice during a normal navigation run:The root cause is that we attempt to get a preview of the
DevtoolsLogandTraceartifacts before running through thegetArtifactphase properly. This doesn't actually cause any problems because thegetArtifactimplementation is idempotent in both cases. However this could theoretically cause problems if that were to change down the line.This PR adds a separate path for the navigation runner to get it's preview of the DT log and trace so that making changes to
getArtifactis safer.