selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[🚀 Feature]: [dotnet] [bidi] Finalize public API to prevent further breaking changes

Open nvborisenko opened this issue 5 months ago • 4 comments

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 in AnyDriver class. Thus AnyDriver object is an owner of the BiDi instance, and dispose BiDi as soon as owner is disposed. Not sure, but I think IWebDriver interface is a great candidate to have this new AsBiDiAsync() 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 it public instead of internal, 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] EmptyResult should be valid return type for all methods instead of void. 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 Result class implements IReadOnlyList<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 create extensions, like .GetTreeAsync().AsContexts(). In any case I propose to remove IReadOnlyList<T>, and more important remove strange json converters for these types. - https://github.com/SeleniumHQ/selenium/pull/16219
  • [x] Commands should return Result instead of one object of this Result. Example: SetCookieAsync return Task<PartitionKey> instead of Task<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(...)) then DoSomethingOptions object 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 BiDi instance 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 avatar Jul 27 '25 16:07 nvborisenko

@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

selenium-ci avatar Jul 27 '25 16:07 selenium-ci

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

nvborisenko avatar Oct 21 '25 20:10 nvborisenko

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.

nvborisenko avatar Nov 19 '25 20:11 nvborisenko

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.

nvborisenko avatar Dec 07 '25 10:12 nvborisenko