Allow the ability to specify custom JsonSerializerOptions

[StevenRasmussen]

Allow the ability to specify custom JsonSerializerOptions when calling the IJSInterop.InvokeAsync<T> methods.

Summary of the changes

• Added 2 method overrides to the IJSInterop.InvokeAsync<T> methods that allow passing a custom JsonSerializerOptions object for both serialization and deserialization of the args and return value.

Addresses #12685

[Joelius300]

Not that I have anything to say here but in my opinion the options parameter should come after the identifier. Also, it might be better to wait for the staff members to give a statement on #12685, they're probably talking about it soon (or have already).

[SteveSandersonMS]

@StevenRasmussen Thanks very much for this! Sorry it's taken so long to get back to you on this. We've been so focused on completing .NET 5 for ages.

We're definitely interested in accepting this change. The core concept of passing a per-invocation JsonSerializerOptions is solid. To turn this PR into a completed on that we could merge, I'd suggest:

• I agree with @Joelius300 that it would be more natural to place the JsonSerializerOptions after the identifier (but before other params)
• The PendingTask class should be changed to a readonly struct
• Since we now also have IJSObjectReference as well as IJSRuntime, the equivalent extra methods with JsonSerializerOptions should be added to IJSObjectReference too
• We would need E2E test coverage for this change, by extending the existing E2E test cases for JS interop

I know it's been a long time since you started this PR. Are you still keen to finish it off so we could merge it? If not that's fine, as we could get someone on our team to do it instead (but there would be no specific ETA for that yet - we're too early in .NET 6 planning).

Also CC @pranavkm who's worked on some JS interop pieces recently in case he has other design feedback.

As a sidenote, I'm also interested in what we could do about calls coming in the other direction (JS to .NET). What would be nice is if the .NET code could receive a JObject (or whatever System.Text.Json calls it) and then call the deserializer itself with its own options. BTW: if there's anything nontrivial for us to do about that, please don't attempt it in this PR - it should be a separate PR.

[StevenRasmussen]

@SteveSandersonMS - Thanks for the feedback! I will work on rebasing and incorporating the changes as you suggest. Re ETE test coverage: I'll need some specific guidance on what you want here. Pointing to some existing tests that I could use as a starting point would be great. Thanks!

[StevenRasmussen]

@SteveSandersonMS - I've rebased and updated per your comments. I noticed that IJSInProcessObjectReference.cs and IJSInProcessRuntime.cs had similar interfaces (with an InvokeAsync method) and so I added the overload to those interfaces as well.

I wasn't sure if we should add overloads for each of the extension methods in the JSRuntimeExtensions.cs. Let me know and I can add those in.

Re ETE testing: I looked at some of the tests and perhaps something similar to the InteropTest that uses the InteropComponent.razor file might work. Any pointers on how to run the test project in VS (or whatever the best approach is to getting it to run) and how to actually run the tests would be great. I understand that Selenium is involved and although I understand at a high level what it does... not really sure how to actually get the tests to run. I'm able to get the Components.slnf file to build using the command line but I'm unable to actual run any project from VS. Any help is much appreciated. Thanks

[SteveSandersonMS]

Thanks for the updates on this. I've reviewed it and think this is a totally viable feature, so thanks for contributing it! There are some further changes that would be needed to this PR, though:

• For any new methods added to the public interfaces (IJSRuntime/IJSInProcessRuntime/IJSObjectReference/IJSInProcessObjectReference), we'd prefer them not to be breaking changes. That is, people whose apps already implement those interfaces (e.g., in unit tests) should not start getting compiler errors. You can make this nonbreaking by providing default implementations that throw NotSupportedException. The message can be something like The type {GetType().FullName} does not support supplying a value for jsonSerializerOptions..
  • As a consequence of this, you can revert all the changes you made to the existing test fakes, since all of those were just working around the breaking change.
• Let's minimize the number of new methods being added to the public interfaces, so that people who do want to implement support for this on other platforms have less work to do. The underlying interfaces don't need an overload that provides JsonSerializerOptions but not CancellationToken - it's enough just to add a single new overload that takes both of those. Then in the extension method classes (JSRuntimeExtensions/JSInProcessRuntimeExtensions/JSObjectReferenceExtensions/JSInProcessObjectReferenceExtensions) you can provide an extension method that accepts JsonSerializerOptions but not CancellationToken (and uses CancellationToken.None when calling the underlying interface method).
• Further overloads will also be needed on the extension method classes, e.g., for the variants of InvokeVoid/InvokeVoidAsync so they can optionally take a JsonSerializerOptions too.
  • I don't personally think it's necessary to have any combinations that take both TimeSpan and JsonSerializerOptions, since we're ending up with so many combinations, and people who really want both of those can use the overload that takes CancellationToken and JsonSerializerOptions, by converting their timespan to a CancellationToken for the call.
• Nitpick: the new PendingTask struct is private, so it doesn't need to use properties. It will be fractionally more efficient for it to expose public readonly fields instead of the properties.
• We definitely need a comprehensive set of E2E tests covering every new method overload that's being added. Please see the existing InteropTest.cs in the E2ETest project. The corresponding sample code being tested is inside the InteropTest directory within test/testassets/BasicTestApp.

I know I'm asking a lot here. However this is the nature of making such a low-level change to a framework where back-compatibility and long-term maintainability are so vital.

Please let me know if any of the above doesn't make sense!

[SteveSandersonMS]

Any pointers on how to run the test project in VS (or whatever the best approach is to getting it to run) and how to actually run the tests would be great.

Sure:

• To run the test app (to interact with it manually), start up src/Components/test/testassets/TestServer. You can either start it from VS (make it the startup project, then Ctrl+F5), or from the command line (dotnet run in that directory).
• To execute the E2E test cases that automate the test app, make sure you've installed the NPM dependencies (go to src/Components/test/E2ETest and run npm install), then you can either do it in VS (build the E2ETest project, then use the Test Explorer pane to run whatever combination of tests you want from that project) or from the command line (use dotnet test in that directory, most likely with --filter because it would take about 30 minutes to run them all).

[ghost]

Hi @StevenRasmussen.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

[user72356]

@StevenRasmussen This got off to an amazing start, it pains me to see that this ended up going nowhere! Is there an existing facility that provides these features at this time? If not, I hope someone can continue the work you started :-)

[ghost]

Hi @user72356. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.