-
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(user-flow): start/end navigation functions #13966
Conversation
eb020e8
to
f81be39
Compare
lighthouse-core/test/fraggle-rock/scenarios/start-end-navigation-test-pptr.js
Show resolved
Hide resolved
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.
+1, but docs?
Yeah docs sound good. @paulirish is #14021 ready to go first? Also, just had an idea. We could add a new smoke test runner for this. I'll do it in a separate PR. |
*/ | ||
async startNavigation(stepOptions) { | ||
/** @type {ExternalPromise<() => void>} */ | ||
const setupPromise = new ExternalPromise(); |
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.
why can't this just be a promise? looking at the extpromise impl i'm not seeing what's special about it…
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.
There's a pattern that the playwright guys used to do (and still do).. and i think it'd achieve what you're going for here…
let fulfill = () => {};
let reject = () => {};
const result = new Promise((res, rej) => { fulfill = res; reject = rej; });
// …
fulfill(continueNav)
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.
That's not any different. It's just encapsulated. I like it.
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.
There's a big amount of indirection here that takes a moment to figure out. I tried to help in my comment suggestion.
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 main goal was to make the code easier to read by reducing the nested promises and such. It was a generic pattern that we might user later, but that's just wishful thinking I guess.
I'm fine going with paul's suggestion here.
We previously discussed this idea 🔒 here. Using some promise/callback BS we can implement both the callback approach and start/end approach for user triggered navigations.