Skip to content
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.

Sign up for GitHub

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

Add IgnoreQueryString parameter to NavLink #40990

Closed
wants to merge 3 commits into from
Closed

Add IgnoreQueryString parameter to NavLink #40990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
879a0d5
Add 'IgnoreQueryString' parameter
igotinfected Mar 31, 2022
9682520
Handle trailing slashes with query parameters
igotinfected Apr 1, 2022
22ab502
Add E2E tests
igotinfected Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Components/Web/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#nullable enable
Microsoft.AspNetCore.Components.Forms.InputRadio<TValue>.Element.get -> Microsoft.AspNetCore.Components.ElementReference?
Microsoft.AspNetCore.Components.Forms.InputRadio<TValue>.Element.set -> void
Microsoft.AspNetCore.Components.Routing.NavLink.IgnoreQueryString.get -> bool
Microsoft.AspNetCore.Components.Routing.NavLink.IgnoreQueryString.set -> void
28 changes: 26 additions & 2 deletions src/Components/Web/src/Routing/NavLink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ public class NavLink : ComponentBase, IDisposable
[Parameter]
public RenderFragment? ChildContent { get; set; }

/// <summary>
/// Gets or sets a flag to indicate whether route matching should ignore the query string.
/// </summary>
[Parameter]
public bool IgnoreQueryString { get; set; }
Comment on lines +47 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if this behavior could potentially be treated as a subset of #18163?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TanayParikh Reading through that issue made me realise this IgnoreQueryString implementation would also ignore the Fragment part of the URI, if specified after a query string, and in other words, this implementation does not ignore fragments if they are the only extension to the base path.

I'd be interested in taking a look at the ShouldMatch approach if that's the direction we want to go in, just let me know ☺️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a new flag for this. This is just a bug/oversight in existing code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a new flag for this

Are you referring to the whole IgnoreQueryString flag, or to some other hypothetical new flag related to @TanayParikh's comment about the hash part of the URL?

If you mean IgnoreQueryString itself, I agree it would be better if we were ignoring the querystring by default. However that would be a breaking change we would need to weigh up carefully.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think matching on the query string would have offered a good behavior before. Query strings are not ordered, so the chances of matching on the query string would have been incredibly narrow.

I think we can just make the change and make an announcement about it if we deeply care about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the default, however there are clear cases where someone might have wanted it to match on querystrings and might in fact be depending on that behavior now. For example, if you have:

@page "/products"

@code {
    [Parameter]
    [SupplyParameterFromQuery(Name = "category_id")]
    public int CategoryId { get; set; }
}

... then you might also have navigation links like:

<NavLink href="products?category_id=1">Dogs</NavLink>
<NavLink href="products?category_id=2">Cats</NavLink>
<NavLink href="products?category_id=3">Fish</NavLink>

... and expect those links to light up to show the current page.

It's not the most common pattern, but it's a valid and believable one so we should be prepared for making a clear announcement and treating it as a breaking change if we're going to change this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'm fine if we do that.

The alternative is to properly support query string parameters, but that is much more work.

I want to avoid the situation where we have a behavior that "half" works and I think in this case it is simpler to not support it at all vs to add full support for it.


/// <summary>
/// Gets or sets a value representing the URL matching behavior.
/// </summary>
Expand Down Expand Up @@ -113,13 +119,15 @@ private bool ShouldMatch(string currentUriAbsolute)
return false;
}

if (EqualsHrefExactlyOrIfTrailingSlashAdded(currentUriAbsolute))
var matchingUri = IgnoreQueryString ? RemoveQueryString(currentUriAbsolute) : currentUriAbsolute;

if (EqualsHrefExactlyOrIfTrailingSlashAdded(matchingUri))
{
return true;
}

if (Match == NavLinkMatch.Prefix
&& IsStrictlyPrefixWithSeparator(currentUriAbsolute, _hrefAbsolute))
&& IsStrictlyPrefixWithSeparator(matchingUri, _hrefAbsolute))
{
return true;
}
Expand Down Expand Up @@ -192,4 +200,20 @@ private static bool IsStrictlyPrefixWithSeparator(string value, string prefix)
return false;
}
}

private static string RemoveQueryString(string path)
{
if (!path.Contains('?'))
{
return path;
}

var pathWithoutQuery = path.Split('?')[0];

// We remove trailing slashes to ensure the following matches:
// - "/abc/?value=123" matches "/abc"
// - "/abc/def?value=123" matches "/abc/def"
// - "/abc/def/?value=123" matches "/abc/def"
return pathWithoutQuery.EndsWith('/') ? pathWithoutQuery.TrimEnd('/') : pathWithoutQuery;
}
}
40 changes: 40 additions & 0 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,46 @@ public void CanNavigateBetweenPagesWithQueryStrings()
AssertHighlightedLinks("With query parameters (none)", "With query parameters (passing string value)");
}

[Fact]
public void IgnoresQueryString()
{
SetUrlViaPushState("/WithQueryParameters/IgnoreQueryString?intvalue=123");

var app = Browser.MountTestComponent<TestRouter>();
Assert.Equal("123", app.FindElement(By.Id("value-QueryInt")).Text);
AssertHighlightedLinks("Base IgnoreQueryString URL");
}

[Fact]
public void IgnoresQueryStringWithTrailingSlash()
{
SetUrlViaPushState("/WithQueryParameters/IgnoreQueryString/?intvalue=123");

var app = Browser.MountTestComponent<TestRouter>();
Assert.Equal("123", app.FindElement(By.Id("value-QueryInt")).Text);
AssertHighlightedLinks("Base IgnoreQueryString URL");
}

[Fact]
public void DoesNotIgnoreQueryString()
{
SetUrlViaPushState("/WithQueryParameters/DoNotIgnoreQueryString?intvalue=123");

var app = Browser.MountTestComponent<TestRouter>();
Assert.Equal("123", app.FindElement(By.Id("value-QueryInt")).Text);
AssertHighlightedLinks("With query parameters (matches all and does not ignore query string)");
}

[Fact]
public void DoesNotIgnoreQueryStringWithTrailingSlash()
{
SetUrlViaPushState("/WithQueryParameters/DoNotIgnoreQueryString/?intvalue=123");

var app = Browser.MountTestComponent<TestRouter>();
Assert.Equal("123", app.FindElement(By.Id("value-QueryInt")).Text);
AssertHighlightedLinks("With query parameters (matches all and does not ignore query string with trailing slash)");
}

private long BrowserScrollY
{
get => (long)((IJavaScriptExecutor)Browser).ExecuteScript("return window.scrollY");
Expand Down
6 changes: 6 additions & 0 deletions src/Components/test/testassets/BasicTestApp/RouterTest/Links.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@
<li><a href="/subdir/images/blazor_logo_1000x.png" download>Download Me</a></li>
<li><NavLink>Null href never matches</NavLink></li>
<li><custom-link-with-shadow-root target-url="Other"></custom-link-with-shadow-root></li>
<li><NavLink href="/subdir/WithQueryParameters/IgnoreQueryString" Match=NavLinkMatch.All IgnoreQueryString=@true>Base IgnoreQueryString URL</NavLink></li>
<li><NavLink href="/subdir/WithQueryParameters/IgnoreQueryString?intvalue=123" Match=NavLinkMatch.All IgnoreQueryString=@true>With query parameters (matches all and ignores query string)</NavLink></li>
<li><NavLink href="/subdir/WithQueryParameters/IgnoreQueryString/?intvalue=123" Match=NavLinkMatch.All IgnoreQueryString=@true>With query parameters (matches all and ignores query string with trailing slash)</NavLink></li>
<li><NavLink href="/subdir/WithQueryParameters/DoNotIgnoreQueryString" Match=NavLinkMatch.All IgnoreQueryString=@false>Base DoNotIgnoreQueryString URL</NavLink></li>
<li><NavLink href="/subdir/WithQueryParameters/DoNotIgnoreQueryString?intvalue=123" Match=NavLinkMatch.All IgnoreQueryString=@false>With query parameters (matches all and does not ignore query string)</NavLink></li>
<li><NavLink href="/subdir/WithQueryParameters/DoNotIgnoreQueryString/?intvalue=123" Match=NavLinkMatch.All IgnoreQueryString=@false>With query parameters (matches all and does not ignore query string with trailing slash)</NavLink></li>
</ul>

<button id="do-navigation" @onclick=@(x => NavigationManager.NavigateTo("Other"))>
Expand Down