-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(fr): separate audit phase for flows #13623
Conversation
@@ -70,6 +70,8 @@ async function waitForImagesToLoad(page) { | |||
|
|||
await flow.navigate('https://www.mikescerealshack.co/corrections'); | |||
|
|||
await flow.endFlow(); | |||
|
|||
const flowResult = flow.getFlowResult(); |
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.
is there a reason to have separate functions for endFlow and getFlowResult?
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 didn't want to make getFlowResult
async but that concern is kinda thin. I'm up for either.
const name = this.name || `User flow (${url.hostname})`; | ||
return {steps: this.steps, name}; | ||
async getFlowResult() { | ||
if (this.flowResult) return this.flowResult; |
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.
Seems like if the user calls getFlowResult, followed by more gathering, then getFlowResult again, they will get a cached result and no indication that something they did was wrong.
Should either guard against that (throw if already called getFlowResult), or just not cache. If the latter, might be good to rename to createFlowResult
.
also nit: rename getUserFlowReport
?
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.
Should either guard against that (throw if already called getFlowResult), or just not cache. If the latter, might be good to rename to createFlowResult.
I'm thinking we remove the cache.
also nit: rename getUserFlowReport?
This function returns a FlowResult
object not a string containing the user flow report.
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 meant getUserFlowResult
.
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 it makes more sense since the object type is FlowResult
. I also think it's pretty clear that "flow" refers to a "user flow" and not some other type of flow. The constructor for UserFlow
isn't exposed on the api either.
/** | ||
* @param {LH.FlowResult} flowResult | ||
*/ | ||
async function generateFlowReport(flowResult) { |
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 added this api function for 2 reasons:
- Allows us to generate the flow result just once when we generate flow fixtures with
--view
flag - We will eventually need a way to generate a flow report for a result object generated from artifacts and not the
UserFlow
class
Step 3 of #13364
Ref
#11313