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.

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

NavigationManager.Refresh does not make use of optional parameter 'forceReload' #59854

Open
1 task done
Nico1395 opened this issue Jan 13, 2025 · 11 comments
Open
1 task done
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@Nico1395
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hey!

I wanted to use NavigationManager.Refresh(forceReload: false) on our .NET8 Blazor Server app and noticed that forceReload is not being passed through to NavigateTo, instead the forceReload of NavigateTo defaults to true. You can easily see what I mean in this file in line 178:

=> NavigateTo(Uri, forceLoad: true, replace: true);

Expected Behavior

I assume this is not intentional as I cannot come up with a reason why it would be. If the parameter exists one would expect it to be used, right?

Steps To Reproduce

The code is obvious, but NavigationManager.Refresh(forceReload: false) reproduces the issue.

Exceptions (if any)

None

.NET Version

8.0.x

Anything else?

I think the issue speaks for itsself.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Jan 13, 2025
@willdean
Copy link

The comment on that method is garbled too.

@javiercn javiercn added bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Jan 21, 2025
@javiercn javiercn added this to the Backlog milestone Jan 21, 2025
@javiercn
Copy link
Member

@willdean thanks for contacting us.

It seems that this is a bug.

@MackinnonBuck was this intentional?

@MackinnonBuck
Copy link
Member

was this intentional?

This is intentional. Classes extending NavigationManager are expected to override the Refresh() method and handle the forceReload argument in an implementation-specific manner.

Since that method was added in .NET 8, and because it's a breaking change to introduce new abstract members to an existing class, we had to make the method virtual and provide a reasonable default behavior that would work with existing NavigationManager implementations.

Note that this method was added in the same release as static server rendering, meaning existing NavigationManager implementations would have been designed for fully interactive contexts, where NavigateTo(Uri, forceLoad: false, replace: true) is effectively a no-op (so you basically never want to do that). To maximize the chance that NavigationManager.Refresh() exhibits the expected behavior on existing implementations, we decided to pass forceLoad: true in the default implementation. Which, if you're calling Refresh() in a fully interactive context, is almost always what you want.

Also note that the XML docs indicate that forceReload: false doesn't prevent a full page reload. It just means "don't force a full page reload":

/// If <paramref name="forceReload"/> is <c>true</c>, a full page reload will always be performed.
/// Otherwise, the response HTML may be merged with the document's existing HTML to preserve client-side state,
/// falling back on a full page reload if necessary.

Meaning it's valid to always pretend the parameter is true.

Hope this clarifies things!

@MackinnonBuck
Copy link
Member

If there's a change to make here, it's probably a comment in the body of the NavigationManager.Refresh() method (not the XML docs!) that clarifies why we pass forceLoad: true. It could just be a short phrase, like:

// In the default implementation, we ignore the forceReload argument and always pass forceLoad: true.
// That way, this method doesn't no-op by default in implementations designed for fully interactive contexts.

@MackinnonBuck
Copy link
Member

MackinnonBuck commented Jan 22, 2025

I wanted to use NavigationManager.Refresh(forceReload: false) on our .NET8 Blazor Server app

@Nico1395, could you clarify what you expected this to do, and how the actual behavior differs from that? I'm guessing this is a Blazor Server app created prior to .NET 8 but upgraded later? Or is it actually a Blazor Web App with server interactivity, created in the .NET 8+ timeframe?


cc @javiercn - I see this was triaged as a bug and moved to the backlog, and I want to make sure there's visibility of the discussion.

@Nico1395
Copy link
Author

Nico1395 commented Jan 29, 2025

@MackinnonBuck Sorry for the late response, I was in my holidays.

Well I would have expected the parameter to be used by default, so I could decide whether I prefer either a 'soft' refresh without a forced reload, or a 'hard' refresh with a forced reload.

However while I do have the rare case where I would like to soft-refresh the page, I agree its not something you need often at all. You can also very easily work around it by NavigateTo the current URI, so no idea whether its worth it spending time on this. I just found it curious and unintuitive the first time I saw it, and therefore thought it was faulty.

@MackinnonBuck
Copy link
Member

No worries at all, @Nico1395.

Well I would have expected the parameter to be used by default, so I could decide whether I prefer either a 'soft' refresh without a forced reload, or a 'hard' refresh with a forced reload.

Thanks for clarifying. I'm guessing by "soft" refresh you mean doing a re-render of the current page? If so, then yes, doing NavigateTo() with the current URI is what you want. But I would imagine it's rare to need to do this (the only scenario I can think of is if there's a hash in the URL and you want to scroll back to the hashed element). Is this your scenario, or is there another reason you need to do this? Just want to avoid closing this issue without understanding the use case.

@garrettlondon1
Copy link

Also, If the page is static with an interactive island, and you call NavigateTo() with false, the InteractiveServer component will also not re-initialize, nor will the navigation happen

@MackinnonBuck
Copy link
Member

Also, If the page is static with an interactive island, and you call NavigateTo() with false, the InteractiveServer component will also not re-initialize, nor will the navigation happen

Right, and that's expected when you pass the current URI to NavigateTo(). If the static re-render produces the same content, nothing will change visually. It's when the static re-render does produce new content that this is useful. Maybe that's what you're calling out (or if you meant that there's a bug here, let me know what that is).

@garrettlondon1
Copy link

garrettlondon1 commented Jan 31, 2025

Yeah @MackinnonBuck , is the solution to add a @key property to the Interactive component on the static page so that it calls OnInitializedAsync in the interactive component again?

TBH, I think passing current URI to NavigateTo() should 100% reload the page, and is currently a bug. All interactive components should be re-rendered, but not generate a new WS connection unless forceLoad = true is passed.

@MackinnonBuck
Copy link
Member

All interactive components should be re-rendered

Meaning re-initialized (they get Dispose()d and then OnInitializeAsync() runs again)? They should already re-render if their parameters change.

Yeah, using a @key should work. May I ask why you need components to completely re-initialize, though? I would imagine that re-rendering the component with updated parameters should be enough for most cases. Our current guidance would be to use forceLoad: true if you need to completely reset the state of the page. Preserving the websocket connection while re-initializing interactive state is an interesting scenario that lies somewhere in between full state preservation and full reload, and getting an understanding of what scenarios require that would be helpful.

But, maybe that discussion should continue in #56961, since this issue tracks something else 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

No branches or pull requests

5 participants