Skip to content

Blazor API Review: HttpClient functionality #11756

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

Closed
rynowak opened this issue Jul 1, 2019 · 7 comments · Fixed by #11767
Closed

Blazor API Review: HttpClient functionality #11756

rynowak opened this issue Jul 1, 2019 · 7 comments · Fixed by #11767
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed

Comments

@rynowak
Copy link
Member

rynowak commented Jul 1, 2019

Blazor API Review: HttpClient functionality

Blazor currently has extension methods on HttpClient for using the new System.Text.Json serializer. Ultimately we want this functionality to be part of CoreFx, and not something that the Blazor team control.

As part of 3.0 we should move this functionality to another package that will keep its preview branding until similar convenience APIs are available from CoreFx. This avoids any naming conflicts within the actual shipping product.

Once the functionality moves to CoreFx, we can encourage users to find it there and stop shipping our package, but it won't break anyone's app or components. Shipped code that that depends on this package will continue working (not a binary breaking change because its already a separate package).

I'm proposing Microsoft.AspNetCore.Blazor.HttpClient as the package/assembly name.

This name will not be used as a namespace since all of the types in the package are in existing namespaces.

APIs

There are no planned changes to the primary API as part of this change - just a package split. Since these APIs remain in preview they are not our focus at this time.

    public static partial class HttpClientJsonExtensions
    {
        [System.Diagnostics.DebuggerStepThroughAttribute]
        public static System.Threading.Tasks.Task<T> GetJsonAsync<T>(this System.Net.Http.HttpClient httpClient, string requestUri) { throw null; }
        public static System.Threading.Tasks.Task PostJsonAsync(this System.Net.Http.HttpClient httpClient, string requestUri, object content) { throw null; }
        public static System.Threading.Tasks.Task<T> PostJsonAsync<T>(this System.Net.Http.HttpClient httpClient, string requestUri, object content) { throw null; }
        public static System.Threading.Tasks.Task PutJsonAsync(this System.Net.Http.HttpClient httpClient, string requestUri, object content) { throw null; }
        public static System.Threading.Tasks.Task<T> PutJsonAsync<T>(this System.Net.Http.HttpClient httpClient, string requestUri, object content) { throw null; }
        public static System.Threading.Tasks.Task SendJsonAsync(this System.Net.Http.HttpClient httpClient, System.Net.Http.HttpMethod method, string requestUri, object content) { throw null; }
        [System.Diagnostics.DebuggerStepThroughAttribute]
        public static System.Threading.Tasks.Task<T> SendJsonAsync<T>(this System.Net.Http.HttpClient httpClient, System.Net.Http.HttpMethod method, string requestUri, object content) { throw null; }
    }

Additional Details

We need to decide what to do with JsonSerializerOptionsProvider. This class provides internal access to a JsonSerializerOptions object that's used by several of our components. This is internal currently and used inside the methods that are moving so we need to make a decision.

using System.Text.Json;

namespace Microsoft.AspNetCore.Components
{
    internal static class JsonSerializerOptionsProvider
    {
        public static readonly JsonSerializerOptions Options = new JsonSerializerOptions
        {
            PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
            PropertyNameCaseInsensitive = true,
        };
    }
}

It seems like these settings are used everywhere Blazor wants to interop with javascript (but not via JS interop). For instance when using InvokeUnmarshalled. I think we could safely make this type shared-source with few drawbacks since it's not mutated and not exposed.


We need to decide how users acquire this package. There are a few options (can be combined):

  • Just publish the package and update docs
  • Make M.A.Blazor depend on new package
  • Add new package to templates (what about RCL? that's an RTW template)

I think it's most important that we get an answer for the RCL question. Given that we have HTTP in our client-side template, we need to show the correct way to do it, which means it's either a dependency of those templates or of M.A.Blazor.

@rynowak rynowak added area-blazor Includes: Blazor, Razor Components API Review labels Jul 1, 2019
@rynowak rynowak added this to the 3.0.0-preview8 milestone Jul 1, 2019
@rynowak rynowak self-assigned this Jul 1, 2019
@rynowak
Copy link
Member Author

rynowak commented Jul 1, 2019

We need to decide how users acquire this package. There are a few options (can be combined):

@danroth27 @SteveSandersonMS - this is somewhat pressing to figure out. My main concern is:

Given that we have HTTP in our client-side template, we need to show the correct way to do it, which means it's either a dependency of those templates or of M.A.Blazor.

@SteveSandersonMS
Copy link
Member

Having it be a dependency of the template seems safer given we explicitly plan to not have it as a dependency of M.A.Blazor forever.

Also when people build a library, I don’t want them to think the right way to get these APIs is by referencing M.A.Blazor simply because doing so makes their compiler errors go away. That’s a time bomb if they do. It’s better if they have no choice but to reference M.A.Blazor.HttpClient from the library.

@rynowak
Copy link
Member Author

rynowak commented Jul 1, 2019

Also when people build a library, I don’t want them to think the right way to get these APIs is by referencing M.A.Blazor simply because doing so makes their compiler errors go away.

This is a good point that I hadn't considered.

Dan and I were discussing in person and hadn't come up with a strong reason to choose between these - Dan has a slight preference for dependency of M.A.Blazor and I had a slight preference for in the template. Now I have a strong preference for in the template.

@danroth27
Copy link
Member

In the template is fine.

@rynowak rynowak added the Working label Jul 1, 2019
@danroth27 danroth27 mentioned this issue Jul 1, 2019
20 tasks
rynowak added a commit that referenced this issue Jul 1, 2019
rynowak added a commit that referenced this issue Jul 2, 2019
@rynowak rynowak added Done This issue has been fixed and removed Working labels Jul 2, 2019
@iAmBipinPaul
Copy link

Blazor is unable to deserialize date time in the case of prerendering but it does work in the case of client-side

(same issue on corefx https://github.com/dotnet/corefx/issues/38568#issuecomment-503405420 solution: add a custom converter)

Error message

The JSON value could not be converted to System.DateTime. Path: $.expiresAt | LineNumber: 0 | 
BytePositionInLine: 96
 var userInfo = await _httpClient.GetJsonAsync<UserInfo1>("user");

Where UserInfo1

  public class UserInfo1
    {
        public bool IsAuthenticated { get; set; }
        public string Name { get; set; }
        public string Email { get; set; }
        public  string Token { get; set; }
        public DateTime ExpiresAt { get; set; } 

    }

how can we plug custom converter for GetJsonAsync? or it is not recommended to use blazor version of HttpClient in the case of prerendering?

@rynowak
Copy link
Member Author

rynowak commented Jul 10, 2019

Hi, it looks like you are posting on a closed issue/PR/commit!

We're very likely to lose track of your bug/feedback/question unless you:

  1. Open a new issue
  2. Explain very clearly what you need help with
  3. If you think you have found a bug, include detailed repro steps so that we can investigate the problem

Thanks!

@iAmBipinPaul
Copy link

Hi, @rynowak
I have opened a new issue with a sample project.
#12046
Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants