-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] [bidi] Stateless converters with hydration #16670
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
7b398a1
b0f5e1e
79f723d
cafbba5
d7956b4
aaddb1e
25389bd
61ad320
2d20a43
3f482c0
00d499e
7204604
a465945
4e487df
1501af1
d1a2fcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,6 +279,11 @@ private void ProcessReceivedMessage(byte[]? data) | |
| var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) | ||
| ?? throw new JsonException("Remote end returned null command result in the 'result' property."); | ||
|
|
||
| if (commandResult is IBiDiHydratable bidiHydratable) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any types with properties that are BiDi hydratable? We might need to handle nested structures.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hydrated couple just to make tests green. If we like the approach, I will review all top-level CommndResults/EventArgs to include nested properties. |
||
| { | ||
| bidiHydratable.Hydrate(_bidi); | ||
| } | ||
|
|
||
| command.TaskCompletionSource.SetResult((EmptyResult)commandResult); | ||
| } | ||
| catch (Exception ex) | ||
|
|
@@ -304,6 +309,11 @@ private void ProcessReceivedMessage(byte[]? data) | |
| { | ||
| var eventArgs = (EventArgs)JsonSerializer.Deserialize(ref paramsReader, eventInfo)!; | ||
|
|
||
| if (eventArgs is IBiDiHydratable bidiHydratable) | ||
| { | ||
| bidiHydratable.Hydrate(_bidi); | ||
| } | ||
|
|
||
| var messageEvent = (method, eventArgs); | ||
| _pendingEvents.Add(messageEvent); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -17,15 +17,15 @@ | |||
| // under the License. | ||||
| // </copyright> | ||||
|
|
||||
| using System.Text.Json; | ||||
| using OpenQA.Selenium.BiDi.Json.Converters; | ||||
| using System.Text.Json.Serialization; | ||||
| using System.Threading.Tasks; | ||||
|
|
||||
| namespace OpenQA.Selenium.BiDi.Browser; | ||||
|
|
||||
| public sealed class BrowserModule : Module | ||||
| { | ||||
| private BrowserJsonSerializerContext _jsonContext = null!; | ||||
| private static readonly BrowserJsonSerializerContext _jsonContext = BrowserJsonSerializerContext.Default; | ||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we choose generated options, then rename -> |
||||
|
|
||||
| public async Task<CloseResult> CloseAsync(CloseOptions? options = null) | ||||
| { | ||||
|
|
@@ -77,11 +77,6 @@ public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorDeniedAsync(SetD | |||
|
|
||||
| return await Broker.ExecuteCommandAsync(new SetDownloadBehaviorCommand(@params), options, _jsonContext.SetDownloadBehaviorCommand, _jsonContext.SetDownloadBehaviorResult).ConfigureAwait(false); | ||||
| } | ||||
|
|
||||
| protected override void Initialize(JsonSerializerOptions options) | ||||
| { | ||||
| _jsonContext = new BrowserJsonSerializerContext(options); | ||||
| } | ||||
| } | ||||
|
|
||||
| [JsonSerializable(typeof(CloseCommand))] | ||||
|
|
@@ -96,4 +91,12 @@ protected override void Initialize(JsonSerializerOptions options) | |||
| [JsonSerializable(typeof(GetClientWindowsResult))] | ||||
| [JsonSerializable(typeof(SetDownloadBehaviorCommand))] | ||||
| [JsonSerializable(typeof(SetDownloadBehaviorResult))] | ||||
|
|
||||
| #pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are CLS compliant:
|
||||
| [JsonSourceGenerationOptions( | ||||
| PropertyNameCaseInsensitive = true, | ||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are duplicating shared json options. Is it good? Or do we want to return back the construction of json context in runtime rather than generated options? |
||||
| DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, | ||||
| PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, | ||||
| Converters = [typeof(DateTimeOffsetConverter)])] | ||||
| #pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant | ||||
| internal partial class BrowserJsonSerializerContext : JsonSerializerContext; | ||||
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 it safe to remove runtime options completely? As I remember we implemented it as converter?..