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

[nvborisenko]

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.
• Really expose all hidden methods from users, such as AddIntercept(). So just make it public instead of internal, that's it. [🚀 Feature]: [dotnet] [bidi] Expose BiDi to be truly public low-level API #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.
  RESOLUTION: Removed them at all to be low-level, extension methods are still in mind.
• EmptyResult should be valid return type for all methods instead of void. [🚀 Feature]: [dotnet] [bidi] Introduce EmptyResult as a type #15562
• Normal types as event args (inheritance topic) [🚀 Feature]: [dotnet] [bidi] Re-think about EventArgs #15546 Solution: introduce non-serializable top-level *EventArgs, and construct it from spec defined type (including IBiDi hydration!) [dotnet] [bidi] Revise event args #17304
• We have more and more strange syntax sugar, like Result class implements IReadOnlyList<T> -

  selenium/dotnet/src/webdriver/BiDi/BrowsingContext/GetTreeCommand.cs

  Line 50 in 55f02a9

  public sealed record GetTreeResult : EmptyResult, IReadOnlyList<BrowsingContextInfo>

  . 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. - [dotnet] [bidi] Remove IEnumerable of command results #16219
• 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. - [dotnet] [bidi] Specific result type for any command #16405
• 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. It is still not safe, we can use records with {get;init;}. - [dotnet] [bidi] Fully immutable commands and events #17077
• 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. [dotnet] [bidi] Stateful converters with hydration #16670
• Scoped event handlers: remove or keep: [🚀 Feature]: [bidi] Multiple scoped event listeners/handlers #16604 [dotnet] [bidi] Remove browsing scoped events at core level #16694
• Expose functionality via interfaces? IBiDi/IBrowserModule? What benefits, testability?
• Discriminated unions are coming back (Add commands for screencasting w3c/webdriver-bidi#1069 (comment)). How to implement?.. Supporting netstandard2.0/net462 target.. History: [dotnet] [bidi] Earlier preview feedback gathering  #14530.
  UPD: Seems unions semantically will be without json discriminator, fuh. So, continue using polymorphic types, just handle this special case manually.
• Extensible command/result/event (2 levels of extensibility: root json object and params object It will not be a breaking change.
• IReadOnly vs IEnumerable. Currently we accept IEnumerable as incoming interface, meaning we can fail with Collection was modified during serialization. This is only one interface for commands execution which is not truly safe. Better way is to accept materialized collections. This is still convenient for users, I believe.
  UPD: seems ImmutableArray is our key point. - [dotnet] [bidi] Immutable user facing collections #17533
• 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).

[selenium-ci]

@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

[nvborisenko]

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]

One more use case to avoid stateful converters: appium/dotnet-client#969 (comment). 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]

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]

Everything is technically resolved.

Last question: driver.AsBiDiAsync() extension method. It should? be part of some interface or we can leave it as is. To answer on this question we should understand deterministically who will support/implement BiDi protocol. Any WebDriver? Or Chromium/Firefox only?

[diemol]

We are now working with ADRs, which should align public APIs. Please check the open ones and see how these changes fit into that one.

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed. Please open a new issue for related bugs.