fix(domworld): fix waitfor bindings (#6766) #6775
Conversation
There was a problem hiding this comment.
Nice detective work @jschfflr! I was not even aware that contexts have both a name and an id. Do you know whether or not it is a bug that names are added to m_activeBindings when using executionContextId but not when using executionContextName?
| @@ -512,9 +512,12 @@ export class DOMWorld { | |||
| const bind = async (name: string) => { | |||
| const expression = helper.pageBindingInitString('internal', name); | |||
| try { | |||
| // TODO: In theory, it would be enough to call this just once | |||
There was a problem hiding this comment.
Would the binding also survive page navigations if we only call it once?
There was a problem hiding this comment.
The binding is added to m_activeBindings in both cases and stays there until the binding is removed by calling the removeBinding cdp command. And whenever a new ExecutionContext is created, it automatically restores the bindings. This works except for the case when the binding is only exposed to a single execution context identified by an id because it will not be able to restore it.
Adding the binding by name will therefore restore it after navigations. I also added a test for that case to make sure that we don't regress.
There was a problem hiding this comment.
Ah, I see. It sounds like addressing contexts by name instead of id is quite appropriate for this use case then. Thanks!
|
Thanks for taking a look, @johanbay! 👋 LGTM with Johan's questions answered |
Please take a look! This should close #6766