-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
refactor: CriClient -> traditional class + async factory #29676
Conversation
3 flaky tests on run #55947 ↗︎
Details:
cypress/e2e/commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit
|
@@ -144,7 +144,7 @@ export type SendDebuggerCommand = <T extends CdpCommand>(message: T, data?: Prot | |||
|
|||
export type OnFn = <T extends CdpEvent>(eventName: T, cb: (data: ProtocolMapping.Events[T][0], sessionId?: string) => void) => void | |||
|
|||
export type OffFn = (eventName: string, cb: (data: any) => void) => void | |||
export type OffFn = <T extends CdpEvent>(eventName: T, cb: (data: any) => void) => void |
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.
🎉
@@ -144,7 +143,11 @@ const maybeDebugCdpMessages = (cri: CDPClient) => { | |||
cri._ws.send = (data, callback) => { | |||
debugVerboseSend('sending CDP command %o', JSON.parse(data)) | |||
|
|||
return send.call(cri._ws, data, callback) | |||
try { |
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 this going to end up masking problems that we may end up having during development? I think I've run into scenarios where I've coded something wrong and it'll throw an error. I worry that might be hard to track down if we just swallow those errors like this.
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.
Changed to re-throw the error on L#150 in 706a1e3
|
||
private cri: CDPClient | undefined | ||
|
||
constructor ( |
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.
Not sure I like converting this to a list of parameters rather than an object. Especially since one of them is a boolean. I tend to like the named parameters better in that case (though I know the tradeoff is the syntactic sugar of declaring them private up front).
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 fair assessment, but nothing should be directly constructing this class - the static async create
method is used, which uses the named parameters pattern. I could mark the constructor private, I think, to enforce this.
if (fullyManageTabs) { | ||
cri.on('Target.attachedToTarget', async (event) => { | ||
if (this.fullyManageTabs) { | ||
this.cri.on('Target.attachedToTarget', async (event) => { |
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.
Would it be worth setting a temp variable here to avoid the ! syntax. Something like:
const localCRI = this.cri
this.cri.on('Target.attachedToTarget', async (event) => {
try {
// Service workers get attached at the page and browser level. We only want to handle them at the browser level
// We don't track child tabs/page network traffic. 'other' targets can't have network enabled
if (event.targetInfo.type !== 'service_worker' && event.targetInfo.type !== 'page' && event.targetInfo.type !== 'other') {
await localCRI.send('Network.enable', this.protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS, event.sessionId)
}
if (event.waitingForDebugger) {
await localCRI.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId)
}
} catch (error) {
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.
hmm. there's actually a funky issue here, on second glance.
If connect()
gets called again in quick succession after the 'Target.attachedToTarget' event fires but before the event stack has a chance to execute this event listener, this.cri
would not be the same cri instance that triggered the event.
I think all references to cri in these callbacks should be via closure like this, to prevent that mismatch - this prevents a regression from the original construction method.
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.
Looks good. I like the clean up!
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
In preparation for investigating and fixing uncaught CRI errors, this refactors
CriClient
to a more traditional class and async factory method. This will make current and future understanding of this section of the code easier to follow, and prepares it for further extraction refactors.Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?