Skip to content

Blazor QueryString enhancements - design proposal #33338

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
SteveSandersonMS opened this issue Jun 7, 2021 · 17 comments
Closed

Blazor QueryString enhancements - design proposal #33338

SteveSandersonMS opened this issue Jun 7, 2021 · 17 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description Done This issue has been fixed
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jun 7, 2021

Summary

Blazor should provide convenient mechanisms for working with querystrings. This is a popular community request: #22388

Motivation and goals

Developers often want to make some component states bookmarkable/linkable. Classic scenarios for this include:

  • Filtering, searching, and navigating
  • Making shareable state without having to write to a database

Example of filtering/searching/navigating:

image
image

Another:

image
image

Example of more general input state being tracked in the URL (technically this uses # rather than querystring but is conceptually the same):

image

To make this sort of thing work, developers need to be able to:

  • Read incoming data from querystrings on arrival
  • Update the query parameters as the user interacts with the UI. This might be:
    • In response to an event, such as clicking a button or dragging an element
    • In response to editing form elements such as <select> boxes, checkboxes, textboxes
    • In response to clicking on a link, such as for pagination links (e.g., <a href="items?page=2">)
  • Receive notification of query parameters changing when the user clicks back/forwards

In all three cases, the developer usually also needs to perform async data access based on the query parameter (whether it's initial state, a change triggered by the user through the UI, or a change due to back/forwards navigation). Ideally a single place for "loading" logic would handle all three without duplication.

Why this is difficult today

Blazor doesn't currently provide any special support for working with querystrings. Technically you can do it, but it's not a streamlined experience:

  • Technically, you can read/write querystring values via manual URL parsing and formatting, but this is inconvenient and error-prone (e.g., figuring out when to encode/decode).
  • Technically, you can detect when navigation changes a querystring (e.g., via NavigationManager's LocationChanged event), but working with events is inconvenient (e.g., remembering to unsubscribe) and results in duplicated code, compared with reacting to navigation in OnParametersSetAsync
  • Technically, you can bind a textbox to a querystring parameter with oninput, but this involves inventing a whole non-obvious mechanism on your own. Unless you're very careful, you'll be likely to create potential keystroke-loss bugs because you have an async binding cycle.

Do querystrings even matter?

You could argue that anything that can be done with querystrings could also be done with the existing route parameter mechanism. That's perhaps one of the reasons Blazor has managed to go so long without special support here. However,

  • Querystrings are convenient if you have multiple optional parameters, since any subset of them can appear in the URL, and in any order
  • Querystrings have an obvious and standard way to escape user-supplied values so that parsing is robust, unlike route segments where people wouldn't be quite sure if or how to escape values
  • Querystrings help make a more explicit distinction between the part of the URL used for @page selection (the path) versus additional parameters (the query). This is an aesthetics thing.

Web developers instinctively want to use querystrings when they are creating some kind of bookmarkable/linkable state within a page, especially when that state is user-constructed rather than just representing a choice among pre-existing entities. For example, it feels far better to have /dotnet/aspnet/issues?search=Blazor+is+too+cool&sort=upvotes than something like /dotnet/aspnet/issues/sortby-upvotes/milestone-notset/search-Blazor+is+too+cool, even though the latter is technically possible.

In scope

The key end-to-end scenario is having a component that tracks aspects of its state in querystring values, and that state automatically updates if the user uses back/forwards or opens a link where the URL contains pre-existing state.

However we can break this down to lower-level pieces (which collectively cover other end-to-end scenarios too):

  1. A convenient way to read and write individual querystring values by name. This should:
    • Be strongly-typed
    • Automatically take care of formatting/parsing (both the whole URL, and converting individual values to/from relevant .NET types)
    • Cause the browser to actually navigate (via client-side navigation) so the URL updates and any applicable navigation events fire
    • Support specifying whether to append to the history or replace the current entry
  2. A convenient way to receive notifications when querystring values change.
    • This should be possible at least in the current @page component. It's not clear we need to support this for arbitrary descendants that aren't even receiving parameters from the router.
  3. A simple pattern for binding a textbox value to a querystring value, e.g., on each keystroke

You might think that item 3 is redundant because it can be constructed from 1 and 2. However, unless we're careful about it, it's very likely that binding would exhibit gotchas or glitches due to the async nature of navigation events on Blazor Server. For example, after a keystroke you append a character, but because navigation hasn't happened yet, the textbox would re-render without your added character, reverting your keystroke temporarily. This is similar to the async @bind loop bug from the early days of Blazor Server (fixed long before the first public release). We need to be sure this works very conveniently with @bind, without weird problems.

Out of scope

  • Querystrings should explicitly not be involved in the router's selection of @page components.

    • This is to match both the current behavior of server-side ASP.NET Core routing, and the historical behavior of most web frameworks (e.g., /folder/somepage.aspx?a=1&b=2 matches folder/somepage.aspx regardless of the querystring).
    • This means you cannot use URLs like products/123?view=details vs products/123?view=reviews to select between two different routable (@page) components. You would need to have a single component matching products/{id}, and then inside that, use whatever logic you like (possibly based on querystring) to render different content or child components.
  • I don't think we need to have any formatting/parsing options other than "invariant culture"

    • Justification: URLs are treated as invariant in other contexts in ASP.NET Core / Blazor. People who want to do something different can always use the "string" APIs and do their own formatting/parsing.

Risks / unknowns

Unknown: What's the most useful way to handle unparseable querystring values? If the developer is asking for a value called page of type System.Int32 but the value is abc, should we throw, or just treat it as "no value supplied"?

Unknown: Should we treat empty-string values as being equivalent to unset/null, or should they be distinguishable? Generally it's good to preserve information, but it's also quite odd to see ?filter= in a URL, forcing developers to write their own special-case logic if they want empty-string values to disappear from the URL completely.

Risk: Since this API looks and sounds like something from ASP.NET Core (server), some developers might mix up the two. TBH I don't really see how there's an actual problem here, since Blazor already has a client-side routing and parameter passing system and this is just a small extension of it.

I haven't been able to think of any other ways that this feature could be misused, cause security issues, or restrict us from other enhancements in the future. If you think of any, please post below!

Examples

Suggestion from #22388:

@code {
    // Supports two way binding
    [FromQuery] int Page { get; set; }
    [FromQuery(Format = "yyyyMMddHHmmss")] string Date { get; set; }
}

TBH I'm not convinced about some aspects of this:

  • Two-way binding:
    • Doing this for something that looks like a [Parameter] property will mislead people, since it's not legal to write to other [Parameter] properties
    • How does the framework even know that the setter got invoked? If we don't control your setter, we don't get to make it do anything, such as update the URL.
  • Supporting formatting options can be out-of-scope as justified in the Out of scope section above

However, I do agree that [FromQuery] itself looks very usable and convenient as a way to receive values.

Another suggestion from #22388:

var page  = int.Parse(navigationManager.GetQuery("page"));
navigationManager.SetQuery("page", page + 1);

I think the get/set APIs look decent but we could improve them with generics and automatic parsing/formatting. Also the SetQuery method needs some way to control append-vs-replace in the history stack.

Another suggestion from #22388:

@code {
    [QueryParameter("q", IsTwoWay=true, ReplaceHistory=true)]
    string Query { get; set; }

    [QueryParameter(IsTwoWay=true)]
    int Page { get; set; }
}

If we were going to support two-way binding on an inbound property like this then I agree this looks good. However this is not the design I'm going to recommend (e.g., because of the issues mentioned above).

Detailed design

I spent quite a bit of time trying to answer questions like "should you be able to control whether setting a query parameter causes a navigation event" and "how can you get updates without having to remember to unsubscribe" and "how can you control whether it invokes your OnParametersSet or not". Then of course there's the challenge of "how can @bind avoid the async binding loop gotcha (losing keystrokes if you type too fast)". There are ugly solutions to all of these, but we want something that feels obvious and minimal.

It turns out that these questions mostly just vanish if we completely separate reading/writing querystring values from receiving notifications. They should be two independent mechanisms.

Receiving querystring values, with update notifications

The natural way to receive initial querystring values, plus notifications if they change, is to treat them exactly like other route parameters. That is, they go onto a [Parameter] property via SetParametersAsync. Benefits:

  • Retains the component's encapsulation (its choice of how to handle incoming values, and even the choice to model parameters as properties at all)
  • Gives you a natural place to do any async I/O, e.g., when a filter criteria changes. Your OnParametersSetAsync is already the right place to do this, and deals with things like "loading" states, asynchrony, error handling, etc.
  • Means you don't have to distinguish "initial values" vs "updated values" if you don't want. Just put your loading logic in one place.

The router can naturally supply component parameters from the querystring, just like it supplies them from URL segments, as long as it knows which querystring parameters a selected component wants to receive:

  • It can't just supply all querystring parameters, as components typically throw if you supply unknown parameters.
  • By omitting unconsumed parameters, we get to avoid triggering re-renders on the page component if the querystring parameter that changed isn't something it even cares about

The best way I can think of identifying which querystring parameters to pass is a new attribute. Consider handling URLs like /category/123?page=4&filter=chicken%20nuggets:

@page "/category/{id:int}"
@code {
    [Parameter] public int Id { get; set; }

    [Parameter] [FromQuery] public string Filter { get; set; }

    // Equally valid syntax:
    [Parameter, FromQuery("page")] public int CurrentPage { get; set; }
}

The benefit of it actually being a [Parameter] property (rather than just using [FromQuery] alone) is that the whole parameter-supplying mechanism will just work following its normal semantics about notifying-only-if-changed. All existing mechanisms already know how to deal with this.

The benefit of having a separate [FromQuery], rather than [Parameter(FromQuery = true)] is that routing/URLs is an independent concept and isn't good to merge into the concept of parameters.

Drawback: It looks as if you can put [FromQuery] on any parameter in any component, but it would not have any effect except on @page components, because the Router isn't the thing supplying parameter values to descendant components. Possible mitigation: Compiler error if you use [FromQuery] in a page without [RouteAttribute]?

Alternative considered

We could specify querystring parameters as part of the route pattern:

@page "/category/{id:int}?page={page:int}&filter={filter}"

@code {
    [Parameter] public int Id { get; set; }
    [Parameter] public string Filter { get; set; }
    [Parameter] public int Page { get; set; }
}

This has the nice benefit that it's obvious that only the @page component can receive the query values. However it also has lots of worse drawbacks:

  • Inconsistent with server-side routing
  • Makes it look as if the query parameters are mandatory and the ordering is specific
  • The use of constraints makes it look as if you could stop a @page from matching if the value isn't parseable

Getting/setting query parameters procedurally

Besides receiving query parameters with update notifications via [Parameter], any other arbitrary code that has access to the NavigationManager should be able to get/set values:

var page = navigationManager.GetQueryParameter<int>("page");

// Or:
navigationManager.SetQueryParameter("fromDate", startDate);

// Or:
navigationManager.RemoveQueryParameter("page");

Get/set take a generic type T (usually inferred for "set" based on the value). Supported types are the same as for route constraints: string, bool, DateTime, decimal, double, float, Guid, int, long. Passing any other generic type results in a NotSupportedException. Unknown: Should we support Nullable<...> for the relevant subset of these too? I guess so.

Get

Returns the value via culture-invariant parsing, if possible. If there's no value or it's unparseable, we return default(T). We only support there being one value for a given query parameter, so we return the first.

An interesting aspect of this is: what do we do if you call SetQueryParameter, and then before that navigation occurs, you GetQueryParameter it back? Do we tell you the value you just set, or the value that's still in the URL? By default, I think we need to return the value you just set, even before it updates in the URL. This is essential for @bind to work sensibly without losing keystrokes due to asynchrony.

However for the edge case where someone really wants to see the value that's still in the URL:

var page = navigationManager.GetQueryParameter<int>("page", ignorePendingNavigations: true);

Note that, on each incoming navigation event, we discard any pending values and from then on only reflect the actual URL state.

Drawbacks: This means the value returned by GetQueryParameter can be temporarily inconsistent with navigationManager.Url. It's for a good reason, and you can opt out. However some people will still get confused about it. A potentially bigger worry is if navigation could be cancelled and then the GetQueryParameter value would remain inconsistent indefinitely. Currently I'm not aware this could happen but we may need to account for it in the future, resetting the temporary state if navigation is actually cancelled.

Set

Does add-or-update, via culture-invariant formatting. If you pass null for a nullable type T, it's the same as "Delete".

Setting a query parameter should ideally preserve the existing parameter ordering, appending at the end if it's a new key.

Setting a query parameter of course also causes an actual navigation. By default, we should append to the history stack, since that's the default for all the JS APIs. However people will often want to replace the existing entry:

navigationManager.SetQueryParameter("fromDate", startDate, replace: true);

Setting query parameters only triggers client-side navigation. We know there's never a need to force a full page load, because by definition, query parameters don't control page selection.

Who receives notification that you did this? The current @page component, but only if it has a corresponding [Parameter, FromQuery]. That's the mechanism for opting into receiving notifications. There will also be scenarios where people have no need to receive a notification because they don't need to trigger anything in OnParametersSet, as the code writing to the query parameter can also perform whatever actions construct the updated state.

If you want to set multiple query parameters at once, use navigationManager.NavigateTo and do your own URL formatting.

Delete

Needed only if T is non-nullable and you want to remove the value entirely. It triggers the same navigation and other effects as SetQueryParameter. It also supports replace.

Two-way binding to a query parameter

The most explicit and low-tech way to do two-way binding to a query parameter is to define your own get/set pair that uses the GetQueryParameter/SetQueryParameter APIs. Because of the behavior of "set" followed by "get" returning the most-recently-written value, @bind will just do the right thing and there won't be any async binding loop bug:

@page "/something"
@inject NavigationManager Nav

<input @bind="FilterBindable" @bind:event="oninput" />

@code {
    // This is optional. Have it only if you want to receive notifications when the value
    // changes (e.g., on back/forward, or user data entry). Notifications trigger OnParametersSetAsync.
    [Parameter, FromQuery] public string Filter { get; set; }

    string FilterBindable
    {
        get => Nav.GetQueryParameter<string>("filter");
        set => Nav.SetQueryParameter("filter", value);
    }
}

Declaring a get/set pair like this is reasonably conventional for controlling what a @bind does. I know some people will ask why they can't just @bind directly to the Filter parameter, but we can't do that (without introducing new magic) because the framework doesn't even know when you call the setter, so can't know to trigger navigation. Benefits of defining your own get/set pair like this:

  • It's obvious how it works
  • It's obvious how to pass other parameters like replace or do custom formatting conversions
  • You can easily cause side-effects before the navigation or synchronously after it (not waiting for the round-trip to the browser, depending on your needs.

For people who don't want to define their own get/set pair, we could offer a built-in API that hides away this detail:

@page "/something"
@inject NavigationManager Nav

<input @bind="@Nav.Query("filter").Value" @bind:event="oninput" />

@code {
    // This is optional. Have it only if you want to receive notifications when the value
    // changes (e.g., on back/forward, or user data entry). Notifications trigger OnParametersSetAsync.
    [Parameter, FromQuery] public string Filter { get; set; }
}

Nav.Query(name, replace=true) would return a readonly struct that's simply a way to call GetQueryParameter/SetQueryParameter so it can be used with @bind. There can also be a generically-typed overload of it for non-string query parameters:

<input type="date" @bind="@(Nav.Query<DateTime>("startdate").Value)" />

Note that due to Razor syntax limitations, it's necessary to surround the expresion with @(...) because of the generic type parameter. People who find this too cumbersome can of course still define their own get/set pair manually.

We don't have to support Nav.Query(name).Value. We could just say you have to declare your own get/set pair. However I suspect the community will gravitate towards Nav.Query(name).Value as a more compact way of doing binding.

Alternative considered

Instead of the [Parameter, FromQuery] property being of primitive types like string, int, etc., we could define a custom type that specificially represents either a single query parameter or the whole dictionary of query parameters. This would then be usable with @bind:

@page "/something"

<input @bind="Filter.Value" @bind:event="oninput" />

@code {
    [Parameter] public QueryAccessor<string> Filter { get; set; }
}

I know this looks very appealing at first glance. It has fewer lines of code than the proposal above and seems more basic. I was pretty keen on going this way for a while, but it just doesn't line up with how diffing works. The core issue is: should QueryAccessor<T> be treated as deeply immutable for diffing purposes?

  • If yes, then changes to a query parameter wouldn't cause the page to re-render, defeating a key goal.
    • It's also pretty confusing to consider it deeply immutable when its .Value property must be mutable, since that's also a core part of its purpose.
  • If no, then every time a router/layout re-renders, that would cause the page to re-render, whether or not any route parameters have changed. This would be a loss of capabilities compared with the existing routing system. Simply declaring that you receive a QueryAccessor<T> would force you to give up on not re-rendering when unrelated things happen in the layout.
  • If we wanted something more sophisticated where it's only considered changed if the corresponding primitive query value has changed since the last render, well, that's outside the scope of what diffing keeps track of today. Potentially we could enhance diffing along those lines in the future, but it would be a big new feature, and query parameter support doesn't warrant such a change (given that we have a simple and basic alternative above).
    • Note that I did try thinking through many possible schemes, for example where it's a reference object whose identity changes if and only if the underlying primitive has changed, or where it's a struct and we cache the boxed instance, but they are all too weird for customers to reasonably understand.

Also, this wouldn't provide any good way to bind to the query parameter value in a non-@page component, unlike the proposal above which does support that without any additional APIs.

Also, syntactically it looks a lot like any component should be able to receive a QueryAccessor<T>, but in fact that wouldn't be possible for anything other than a @page component. Changing this means reinventing something like cascading parameters but special-cased for routing. It would be better to let developers manage this flow in their own code, like they do for other route data. This is worse than [FromQuery] because there's nothing clearly indicating the developer intends to get the value from routing as opposed to just passing through a QueryAccessor<T> from a parent.

In summary, I don't think [Parameter] QueryAccessor<T> is a good design today, but is something we could consider additively in the future if we choose to make the diffing system have some additional powers that would be overkill for this alone.

Appendix: Async binding loop bugs

In a number of places above I've cited the hazard of "async binding loop bugs". Here's an explanation.

We must not recreate the async 2-way binding bug that long-ago affected Blazor Server (in alpha builds, before the first release). This happens if a component both reads and writes some state in an async binding cycle, e.g.:

  • On keystroke in textbox, set updated query value
  • On query value updated, set updated textbox value

The issue is if keystrokes arrive faster than this cycle completes, they can get lost:

  1. Component reads initial query param value abc and sets it to be the initial state of a textbox
  2. User presses d, quickly followed by e, in that textbox
  3. Because of the first keystroke, .NET receives an event saying the textbox contains abcd
  4. .NET sets some model state to abcd
  5. .NET renders, and diffing updates the textbox to contain abcd (losing the e that was already typed)

Even if the abcde event later gets processed and the UI updated, it's not OK to temporarily undo keystrokes in the UI. Also if they type f while the textbox is temporarily showing abcd, .NET will receive a message saying the textbox contains abcdf and will permanently lose the e.

The @bind mechanism overcomes this by eliminating step 5. It specifically knows that a certain event handler represents an update from the value of a certain element, so pre-patches the "last rendered" tree to contain what we know must already be in the browser-side DOM. Then the diff algorithm sees it as not-a-change and hence there is no step 5.

@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description labels Jun 7, 2021
@SteveSandersonMS
Copy link
Member Author

cc @dotnet/aspnet-blazor-eng

@SteveSandersonMS
Copy link
Member Author

See also: prototype implementation at https://github.com/dotnet/aspnetcore/compare/stevesa/querystring-prototype

@campersau
Copy link
Contributor

Would it also be possible to get / set multiple values for one querystring name?
E.g. mapping ?id=123&id=456 like this

    string[] Ids
    {
        get => Nav.GetQueryParameter<string[]>("id");
        set => Nav.SetQueryParameter("id", value);
    }

@SteveSandersonMS
Copy link
Member Author

@campersau We could support that, but it might be something we choose to leave as a possible future enhancement because you could always do it yourself by (for example) comma-separating the values or using some other encoding.

@mkArtakMSFT mkArtakMSFT added this to the 6.0-preview6 milestone Jun 7, 2021
@ghost ghost added the Working label Jun 7, 2021
@pranavkm
Copy link
Contributor

pranavkm commented Jun 7, 2021

The design looks great to me. I would get someone more familiar with routing (@javiercn / @captainsafia) to vet all of the use cases, but this feels very compelling. A couple of notes:

  • If you pass null for a nullable type T, it's the same as "Delete".
    This feels iffy. Is there any reason to special case this as opposed to simply letting it be an empty value?

  • FromQueryAttribute - I think @davidfowl is still bitter about us re-using RouteAttribute, so this feels like a repeat. Could we name it QueryParameter while still requiring the ParameterAttribute e.g.

[Parameter][QueryParameter] public string Id { get; set; }

Also open to other names.

  • I feel like QueryBindable<T> (the value returned by Nav.Query) could be left as an exercise to users. It's not really hard to write the getter-setters. Having the helper would mean we'd have to maintain 2 sets of overloads if we ever add more options to NavigationManager

  • I only skimmed the async update issue, didn't understand the solution too well, but I'll trust your expertise on it 👍🏽

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jun 7, 2021

FromQuery / QueryParameter

This aspect of naming is awkward. There probably isn't an ideal solution. Other considerations:

  • It would be good to have a name that indicates the one-directional nature of consuming the value, to mitigate the tendency (that people have already posted on the original issue) of thinking they can two-way bind to the inbound parameter. FromQuery is ideal from that point of view.
  • You could argue that having a different name would also be confusing in a different way, as people may try to use [QueryParameter] in MVC.

My main concern would be around the discoverability through intellisense. If VS is likely to show both options, or the wrong option, in code completions, then we have a bad situation. But if in realistic/default cases it only prompts with the right one then I think it would be fine.

Another option would be unsealing ParameterAttribute and then having a FromQueryParameterAttribute inherited from it. This might be fine or might lead to weird effects in tooling or the Razor compiler since they might have any number of assumptions that ParameterAttribute doesn't get subclassed, and would certainly complicate things in the future like using a source generator to produce the parameter assignment logic.

I feel like QueryBindable (the value returned by Nav.Query) could be left as an exercise to users

I'm fine with keeping it out of scope initially. It's achievable in user code as an extension method.

@captainsafia
Copy link
Member

Thanks for writing this up. Looks sensible overall.

However for the edge case where someone really wants to see the value that's still in the URL:

Is this worth addressing? My concern is that introduces an extra layer of complexity to the API. My inclination here is to treat the value the user sets as the source of truth and avoid the flag in the first iteration of the API.

This feels iffy. Is there any reason to special case this as opposed to simply letting it be an empty value?

Hmmm. It actually makes sense to me with the mental model that I've built of this. Nulling out a query param effectively unsets it.

I'm fine with keeping it out of scope initially. It's achievable in user code as an extension method.

Seems sensible to me. I'd imagine an initial implementation would have a:

  • FromQuery attribute
  • SetQueryParam API with support for replace flag
  • Ability to use get/set for two way binding

navigationManager.GetQueryParameter

Should this be TryGetQueryParameter?

@javiercn
Copy link
Member

javiercn commented Jun 8, 2021

For me, I would distill this in two levels:

  • A convenient way to get and set query parameters.
  • A way to react to the URL changing and update the UI if needed based on query parameters changing.

For the first case, we need APIs to provide a way to parse the URL query parameters and to generate a new URL. In this vein, it might make sense to have a type for this, rather than APIs directly on NavigationManager. This is similar to how is done in ASP.NET Core and offers more flexibility:

  • You can get the query parameters object via constructor with new QueryParameters(nm.Url);
  • The object can have all the operations you want to change parameters there.
  • You can make changes to the object and reflect them atomically on the Url by calling by doing nm.NavigateTo(query.BuildUrl(nm.Url)).
  • You can optionally have extension methods on NavigationManager to do these things in a more convenient way:
    • nm.GetQueryParameters()
    • nm.SetQueryParameters(query, replace: true)

For the second part, there are already ways to achieve this in navigation manager:

  • You should be able to subscribe to OnLocationChanged and get notifications when the URL changes.
  • If you had a type for query parameters, you should be able to compare two instances and then react appropriately if they are different.

I'm concerned with us getting out of our depth to solve the async binding issue. I worry that we are tackling on a lot of complexity here to deal with quite an edge case scenario. I strongly suggest we avoid this given how narrow the application is (Binding to query string parameter within an OnInput event) and instead we provide a sample for people who need to do this. If you do OnChange (regular binding) this wouldn't be a problem since there's no "high-frequency" updates.

So to summarize, here are my suggestions:

  • Factor all query string related methods into its own type and API instead of using methods on NavigationManager and have APIs to set the query string as a whole on the NM or to build a URL and call navigate to. We probably want both, otherwise we can't build query strings for other URLs easily. We should support what you can do with query strings and avoid imposing limitations (like returning just the first element).
  • Consider a primitive way components can use to detect query string changes without having to rely on the router or any other component providing parameters as a building block. If we have a type, we can compare for equality or offer a way to check only a subset of keys we are interested in.
  • Avoid dealing with @bind scenarios all-together. It sounds tempting because is very powerful yet simple, however its likely something you end up not using frequently, and that if you do, can abstract within its own component for reuse.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jun 9, 2021

Thanks for the feedback. Also @javiercn and I spent some time talking through lots of details of this and the pros/cons of offering different levels of specialised built-in support.

The whole thing is quite unsatisfying because all the nice solutions are invalidated by async-state-losing gotchas (on Blazor Server). There seems to be only two broad types of solution:

  1. Don't really solve that in the framework at all - leave it up to the developer. They will need to either not use @bind (which leads to other gotchas) like this, or write a bunch of non-obvious code including manually dispatching exceptions, calling StateHasChanged, etc. like this
  2. Solve it in the framework via somewhat specialised magic. It leads to reasonably clean developer code (like this or this), but adds to framework internal complexity and risks being over-coupled to this particular UX pattern.

If we choose to go for the "solve it in the framework" design, the implementation breakdown looks a bit like this:

  • [1 day] Support for "replace" on NavigationManager.NavigateTo
  • [1 day] [FromQuery] or similar way to have router or maybe RouteView pass in additional params from query
  • [1 day] Updating querystring: Get/Set/Delete APIs, possibly also some way to update/delete multiple atomically as a dictionary (needs a bit more design)
  • [0.5 days] Option for NavLink to ignore querystring when matching (like this)
  • [1 day] To make binding seem natural and bug-free:
    • "Get" API to reflect back last "Set" call even before navigation occurs
    • A way to reset this temporary state only after non-programmatic navigation (back/forwards), hence a way to know whether a given navigation even was programmatic or not. Alternative: Set/Delete APIs to update the URL without causing navigation notification from the client, but still trigger the .NET side of navigation notification (short-circuiting the process).
  • [1 day] Possible: <QueryBinder> component to allow hooking up arbitrary non-routed components to query notifications

Add on 50% for contingency and further design changes/reviews, and this is a substantial investment (which still leaves us with a pretty specialized solution).

@dotnet/aspnet-blazor-eng We need to decide if this meets the bar for 6.0 with this cost/benefit in mind. We could:

  • Do it all now
  • Do nothing until 7.0 after collecting more evidence for the scenarios
  • Decide we just want to do (say) the first three items on that list, which would leave the async binding problem for developers to solve, so they would have to write code like this. Note that if we don't do the "Get" API to reflect back last "Set" call even before navigation occurs thing initially, it's hard to do it in the future as it would be breaking change, or require a new set of confusingly-similar APIs.

@SteveSandersonMS
Copy link
Member Author

Also ping @DamianEdwards: don't know if you have time to read all 50 pages or whatever we have here, but you were interested in getting this feature and might have views on the scenarios/scope.

@DamianEdwards
Copy link
Member

Great write up! This was one of the very first issues I hit when writing my first Blazor Server app, as the main UI for the app was an open search text field. As pointed out, I wanted the search terms and other parameters to be preserved in the querystring as is typical in web search UX for the usual reasons.

The code example @SteveSandersonMS links to as an example of what the user would have to do if we only do the first three items in the list he stated is effectively similar to what one has to do today, but vastly more simple and first-class looking. Of course ideally we would make interacting with the querystring as UX as first-class/elegant as other parameters but given the cost involved I'd take that suggestion as a great "round one" improvement for .NET 6 if we can, leaving open the possibility to further refine the experience with higher-level concepts in a future release.

@javiercn
Copy link
Member

  • [1 day] Support for "replace" on NavigationManager.NavigateTo

Could we reuse #25752 we already seem to have an abandoned PR with all the work from a community member. Could we build on top and acknowledge the contribution?

@javiercn
Copy link
Member

  • [1 day] Updating querystring: Get/Set/Delete APIs, possibly also some way to update/delete multiple atomically as a dictionary (needs a bit more design)

Could we copy the design and implementation from https://github.com/dotnet/aspnetcore/blob/main/src/Http/WebUtilities/src/QueryHelpers.cs
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/QueryCollection.cs

Simple tweaks here should suffice to remove the stuff that is specific to HttpAbstractions and make all get/set/replace, etc. operations possible.

@javiercn
Copy link
Member

  • 1 day] [FromQuery] or similar way to have router or maybe RouteView pass in additional params from query

I'm fine if we do this internally inside RouteView, although I think there's no much difference with putting it on a Separate/Reusable component, which will benefit anyone that wants to use query strings independent of the context.

@SteveSandersonMS
Copy link
Member Author

Could we reuse #25752 we already seem to have an abandoned PR with all the work from a community member.

Yes, I hope so, and don't see any reason why not.

Could we copy the design and implementation from ...

Certainly many of the details from that code should be usable/copyable (e.g., all the stuff about encoding/decoding). As for whether updating a single query param should involve building a whole dictionary and then re-encoding everything back into a string, I guess it depends on how much we're interested in optimizing it. I can think of reasonable cases where you might be doing this a hundred times per component render (e.g., links in a grid), so perf is at least worth thinking about.

Maybe the starting point should be an API shape that's reasonably agnostic to the implementation and then we can maybe start with something like the code there, and let it grow into something more optimal in the future if sufficient need arises.

I'm fine if we do this internally inside RouteView, although I think there's no much difference with putting it on a Separate/Reusable component, which will benefit anyone that wants to use query strings independent of the context.

Definitely agree that both options are good to have on the list. I split out the <QueryBinder> concept into a separate potential work item because it's genuinely a whole extra layer of thing to design, and after looking into implementation I'm not sure that we'd want to use the <QueryBinder> internally within RouteView anyway since RouteView could achieve what it needs in literally a few lines of code with less runtime overhead, e.g., without an extra layer in the component graph.

@Thaina
Copy link

Thaina commented Jun 16, 2021

On GetQueryParameter and SetQueryParameter I think it should always be nullable for struct. Any missing or unparseable will be null instead of default. SetQueryParameter with null is removing

Or maybe have the same old bool TryGetQueryParameter<T>(string s,out T value). But really, I think nullable should be common

GetQueryParameter might also have overload for default. That was another way we can easily bypass generic parameter and get default value at the same time

T GetQueryParameter<T>(string s,in T defaultValue)

void DecreasePageNumber()
{
    var page = nav.GetQueryParameter("page",-1);
    if(page < 0)
        return;

    if(page == 0)
        nav.SetQueryParameter("page",null); // remove
    else nav.SetQueryParameter("page",page - 1);
}

While nullable get is still required when we need to distinguish logic between set as default and really missing

On the generic ambiguity of nullable. I commonly design my get related API to always have 3 similar methods

T GetQueryParameter<T>(string s) where T : class // cannot be used to get struct
T GetQueryParameter<T>(string s,in T defaultValue)
T? TryGetQueryParameter<T>(string s) where T : struct // cannot use class to get nullable

@SteveSandersonMS
Copy link
Member Author

All that remains now is #34115, so closing this.

@ghost ghost added the Done This issue has been fixed label Jul 8, 2021
@ghost ghost removed the Working label Jul 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

8 participants