Skip to content

Allow navigation without triggering OnParametersSet #25767

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 Sep 10, 2020 · 15 comments
Closed

Allow navigation without triggering OnParametersSet #25767

SteveSandersonMS opened this issue Sep 10, 2020 · 15 comments
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@SteveSandersonMS
Copy link
Member

This was requested by @zbecknell following a different comment:


We're coming up to about a year of developing a Blazor app for a new project with a small team, and it's amazing! Thanks to this, we've encountered a good number of scenarios with a variety of challenges. As an example scenario I have a page with the following route:

/account/{accountId}

When on this page, there is the possibility of navigating from say /account/1 to /account/2 via NavigationManager -- this causes OnParametersSet to trigger, but NOT OnInitialized since the page is already loaded, so we need to rely on OnParametersSet to know when to reload our page state for a different account.

We want to assume that OnParametersSet triggering really means that a page parameter has changed, otherwise we'd need to keep track of each parameter's before and after state and manually allow the data to reload.

This is why I want to bypass the LocationChanged event, BUT I still need my NavigationManager.Uri to stay synced with the actual browser's URL, otherwise I'd need to use JsInterop to retrieve the actual URL, which necessarily needs to be async, which introduces yet more timing complexity. (I can't use the sync JsInterop because I dual-host client/server Blazor.)

I'm already using interop for my current solution, but it has a bug due to the Uri not being in sync. I'm probably going to solve this in the interim by keeping track of my own Uri variable instead of relying on NavigationManager.


All that being said, this is why another proposal of mine was just letting me set the Uri for NavigationManager programmatically to manually keep it in sync, but I get why that would be likely to cause confusion, and thus isn't an acceptable solution.

@zbecknell
Copy link
Contributor

Thanks, @SteveSandersonMS! Once #25752 gets merged I can work on submitting a PR for this issue.

@SteveSandersonMS
Copy link
Member Author

We want to assume that OnParametersSet triggering really means that a page parameter has changed, otherwise we'd need to keep track of each parameter's before and after state and manually allow the data to reload.

To be honest, having your component keeping track of the navigation parameters, so that OnParametersSet can tell whether they have changed, seems like a more complete solution to me. That gives you the power to make fine-grained decisions about what changes to each one mean, and whether in each case you want to re-render the page component.

If instead the URL and NavigationManager state just changed without the Router re-rendering, this would lead to some very difficult consequences due to how the information flows through the component hierarchy in general. For example, if your host page later re-rendered itself for an unrelated reason, then the updated parameters would flow through the component graph and your OnParametersSet would fire as if a navigation had just occurred.

@zbecknell
Copy link
Contributor

In my case there are no [Parameter]s changing, instead I have a separate component called a PageStateManager that can change and report changes to query parameters.

Allowing my PageStateManager to pass special arguments to NavigationManager.NavigateTo is a one-stop solution, whereas keeping track of parameter state before/after OnParametersSet would require new work on every single page that fits this sort of pattern, which would be error-prone if someone forgets to implement or implements it improperly.

@MariovanZeist
Copy link
Contributor

@zbecknell if I understand your issue correctly, I have an open PR that might help you. It introduces a LocationChanging event, with the possibility to cancel that change.

Although I can't promise my PR will be accepted, and if it is, it probably won't be available until version 6.0 , I am wondering if that would help with your issue because that way you can intercept Uri changes
The PR I mean is #24417.

@SteveSandersonMS
Copy link
Member Author

In my case there are no [Parameter]s changing, instead I have a separate component called a PageStateManager that can change and report changes to query parameters.

Perhaps there are opportunities for PageStateManager to track what has changed.

I agree that it may seem simpler at first glance to have a "don't notify" feature in NavigationManager but I'm not sure how this would be achieved end-to-end. The way the Router works is to supply all the route parameters to the page each time the Router renders. It doesn't specifically know whether it's doing so in reaction to a navigation event or for some other reason. Further, given that we now have asynchronous navigation (to support lazy loading) I'm unsure how this would impact the range of scenario combinations, for example if it happens in the middle of while some other async navigation is happening.

If you want to have a go at considering a possible implementation we could certainly try to think it through more then. We'd need it not to add significant extra complexity or gaps in the abstraction since this has not been a commonly requested feature.

@SteveSandersonMS
Copy link
Member Author

I hope this seems reasonable, by the way. I'm not suggesting your scenario isn't important, it's just that we have to balance the needs of a large community! If there is a really clean way to do it then we could certainly consider it.

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Sep 10, 2020
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Sep 10, 2020
@ghost
Copy link

ghost commented Sep 10, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@zbecknell
Copy link
Contributor

Thanks @MariovanZeist! While that will be a very nice feature to have, in my case I need to allow the Uri to update in the NavigationManager, which I don't believe would happen in the scenario where nav is fully canceled.

@SteveSandersonMS Perfectly reasonable! My position is certainly one of only a surface knowledge of the inner workings, so my request may not align well with what reality would entail. For now, I can probably just manually keep track of the Uri in my PageStateManager and all will be well. If I can devote some time to proposing a solution I may do that, but as you say we won't know how that would impact all the different scenarios, and it'd be unlikely to be simple to test against.

One question I've been meaning to ask: is there any dogfooding planned/announced for Blazor yet? E.g. Fluent UI components are used in Azure DevOps and I'm sure many other places. I know it may be too early, but I'm curious if there's anything either in the works or out there in early stages? (Understandable if this isn't disclosable.)

@MariovanZeist
Copy link
Contributor

@zbecknell You have the ability to cancel it, but if you don't have to. You could "abuse" the location changing event, to parse the Uri before it would trigger a pageload and update your component.

@zbecknell
Copy link
Contributor

@MariovanZeist It's not that the Uri is unknown at the point of it changing -- I instigated the change so I know it at that point -- it's about keeping the NavigationManager.Uri in sync when I do a replaceState, but not triggering events normally associated with changing that Uri. In the case of canceling, my browser URL and Uri would not have the desired changes I want.

I think a manual solution is going to work for me for now. And heck, maybe there's some elegant solution to the OnParametersSet problem that I'm missing, and will hopefully discover over time. Looking forward to seeing how this all evolves. Even being on the bleeding edge it's been a great development experience!

@SteveSandersonMS
Copy link
Member Author

One question I've been meaning to ask: is there any dogfooding planned/announced for Blazor yet?

@danroth27 What public info can we give about internal Blazor usage?

@zbecknell
Copy link
Contributor

@SteveSandersonMS I do now have a workaround that "gets the job done" by inspecting a flag when OnParametersSet is called. BUT, it's not optimal.

Why?

Because it introduced a performance hit. There is a noticeable delay when clicking from one tab to the other due to the extra rendering going on because the full LocationChanged event is triggered.

My other option of attempting to keep track of the Uri myself had a flaw that makes it unworkable. I had another fallback that I considered, which is to try and implement my own NavigationManager (since Uri has a protected set), but this wasn't possible due to internal dependencies.

So at this point, I hope you will consider allowing something like #25782!

@SteveSandersonMS
Copy link
Member Author

There is a noticeable delay when clicking from one tab to the other due to the extra rendering going on because the full LocationChanged event is triggered.

Not certain if this works in your case, but it may be possible to override ShouldRender in your component to suppress the unwanted rendering.

@zbecknell
Copy link
Contributor

That actually seems to fix the performance issue, @SteveSandersonMS! I thought about doing it before, I swear, and I should've tried it! Thanks very much for your help with this. I still hope to follow through with a PR for this, but it looks like I've got a good workaround in place for the time being.

So for anyone coming across this and wondering what I've ended up with for now:

I have a PageStateManager with the following method:

private void UpdateCurrentUrl(string url)
{
	// This tells our BaseBusinessPage to ignore the OnParametersSet event firing
	ShouldIgnoreParametersSet = true;

	try
	{
		_js.InvokeVoidAsync("Blazor.navigateTo", url, false, true);
	}
	catch (Exception)
	{
		// We have this here in case we attempt to call the Js stuff before interop is ready in server mode... this is an acceptable fallback
		// The only difference is that it will add an entry to browser history
		_navManager.NavigateTo(url);
	}
}

I have a base page which has PageStateManager injected, in which I do this:

protected override bool ShouldRender()
{
	if (PageState.ShouldIgnoreParametersSet)
	{
		return false;
	}

	return base.ShouldRender();
}

protected async override Task OnParametersSetAsync()
{
	if (PageState.ShouldIgnoreParametersSet)
	{
		// HACK: OnParametersSet is actually called twice, so if we don't delay here, it'll still follow through with the remaining code
		await Task.Delay(1);
		PageState.ShouldIgnoreParametersSet = false;
		return;
	}
        ...
}

@SteveSandersonMS
Copy link
Member Author

Glad you got a solution here. Since this produces the result you want, without weakening or complicating the NavigationManager concept, it seems like a better outcome than adding a further special-case feature, so I'll close this.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants