-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New Blazor querystring building APIs. #34813
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
Conversation
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
src/Components/Components/src/Routing/QueryParameterNameComparer.cs
Outdated
Show resolved
Hide resolved
public bool Equals(ReadOnlyMemory<char> x, ReadOnlyMemory<char> y) | ||
=> x.Span.Equals(y.Span, StringComparison.OrdinalIgnoreCase); | ||
|
||
public int GetHashCode([DisallowNull] ReadOnlyMemory<char> obj) |
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.
Weird you had to do DisallowNull
. Is that part of the signature?
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.
This implementation was pulled out of QueryParameterValueSupplier.cs
, but also I implemented my own comparer before doing that, and iirc it auto generated the DisallowNull
attribute. Found this discussion about it: dotnet/runtime#30998.
Another thing - there isn't implemented support for having |
If it doesn’t blow up the complexity of implementation to support adding multi-valued parameters as part of a dictionary, I think it would be beneficial. Do you think it would work out? |
@SteveSandersonMS I've just added support for multi-valued parameters in |
Made QueryParameterSource<TValue> readonly.
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.
This looks awesome! I think you've done a great job of making such a large number of overloads manageable, and came up with some great abstractions to simplify the shared code in an efficient way.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
New Blazor querystring building APIs.
Added new APIs for building URLs with querystrings from one or multiple parameter names and values.
PR Description
I've added all three APIs documented here. The current implementation doesn't make a massive effort to avoid allocations, but that is something I've been investigating along the way. If we deem it unnecessary to optimize further, I'm fine with the implementation strategy currently in place.
Addresses #34115