[🚀 Feature]: [dotnet] [bidi] Finalize public API to prevent further breaking changes
Description
Let's see how confident we are about BiDi API. In general I like the applied approach, but with some concerns.
- [ ]
driver.AsBiDiAsync()extension method should be finally live inAnyDriverclass. ThusAnyDriverobject is an owner of theBiDiinstance, and disposeBiDias soon as owner is disposed. Not sure, but I thinkIWebDriverinterface is a great candidate to have this newAsBiDiAsync()method. It is important. Finally we will have straightforward dependency:Selenium.Webdriver->Selenium.WebDriver.BiDi. - [x] Really expose all hidden methods from users, such as
AddIntercept(). So just make itpublicinstead ofinternal, that's it. https://github.com/SeleniumHQ/selenium/issues/15612 - [ ] Revisit
InterceptRequestAsync()method (and neighbors: response, auth). This method(s) provides more convenient API to intercept network. Literally saying it should be extensions built on top of already publicly available methods. UPD: .NET 10 provides new extensions capabilities, where we can create properties. Amazing, so the decision here would be move all helpers/extensions to dedicated namespace. - [x]
EmptyResultshould be valid return type for all methods instead ofvoid. https://github.com/SeleniumHQ/selenium/issues/15562 - [ ] Normal types as event args (inheritance topic) https://github.com/SeleniumHQ/selenium/issues/15546
- [x] We have more and more strange syntax sugar, like
Resultclass implementsIReadOnlyList<T>- https://github.com/SeleniumHQ/selenium/blob/55f02a983fb3061ff84020ffe945ed60ee948e33/dotnet/src/webdriver/BiDi/BrowsingContext/GetTreeCommand.cs#L50. It is useful definitely, but seems it is overkill. We can createextensions, like.GetTreeAsync().AsContexts(). In any case I propose to removeIReadOnlyList<T>, and more important remove strange json converters for these types. - https://github.com/SeleniumHQ/selenium/pull/16219 - [x] Commands should return
Resultinstead of one object of thisResult. Example:SetCookieAsyncreturnTask<PartitionKey>instead ofTask<SetCookieResult>. Review all commands and return exact type. - https://github.com/SeleniumHQ/selenium/pull/16405 - [x] Command options should be immutable? Like when I do
await bidi.DoSomethingAsync(new DoSomethingOptions(...))thenDoSomethingOptionsobject should be constructed once and I should not be able to change its state. - Internally we capture command options into immutable object, it is safe that user might change his own options. - [x] Remove stateful json converters (who is dependent on bidi object)! There is no big benefit. - We nicely resolved it technically, still exposing
BiDiinstance for extensibility, and still perfectly AOT trimming friendly. https://github.com/SeleniumHQ/selenium/pull/16670 - [x] Scoped event handlers: remove or keep: https://github.com/SeleniumHQ/selenium/issues/16604 https://github.com/SeleniumHQ/selenium/pull/16694
- [ ] Anything else?..
Have you considered any alternatives or workarounds?
This issue is for tracking possible big changes in the approach. As soon as we resolve all of them, then we are confident with the approach and can move further (like documenting and fine tuning).
@nvborisenko, thank you for creating this issue. We will troubleshoot it as soon as we can.
Selenium Triage Team: remember to follow the Triage Guide
Remove stateful json converters (who is dependent on bidi object)! There is no big benefit.
Let's discuss it.
Original idea:
bidi.Network.OnBeforeRequestSent(e => Console.Write(await e.GetContentAsync()));
Here GetContentAsync() may be extension method. To make it possible e object should expose captured BiDi object. Honestly this is only one use case I can image, probably there are more.
What other use cases might be? Please help to understand why I introduced stateful json converters at the beginning.
CC: @RenderMichael @YevgeniyShunevych
One more use case to avoid stateful converters: https://github.com/appium/dotnet-client/pull/969#discussion_r2543407229. User knows BrowsingContext as string. Currently we cannot convert from string because of BiDi object is required (state).
@RenderMichael you mentioned that we can easily remove stateful converters. Highly appreciated to see how.
Stateful converters are still in place, we technically resolved hidden issues in AOT world. So it is still helpful to expose BiDi instance from DTO types, allowing to extend these types by any user.