dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.44k stars 10.02k forks source link

making StateHasChanged virtual #5674

Open grahamehorner opened 5 years ago

grahamehorner commented 5 years ago

https://github.com/aspnet/Blazor/pull/1716

mkArtakMSFT commented 5 years ago

@SteveSandersonMS do we want to do anything about this?

ajruckman commented 5 years ago

I recently had a use for this and found this issue. I could also see it being useful for debugging, to trace what called StateHasChanged.

Stamo-Gochev commented 4 years ago

@SteveSandersonMS any updates on that?

zHaytam commented 4 years ago

Any updates on this?

mkArtakMSFT commented 4 years ago

Just wanted to drop a note here about the state of this issue, so people understand our current position here. We've moved this issue to the Backlog milestone. This means that it is not going to happen for the upcoming release. We will reassess the backlog following the current release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

grahamehorner commented 4 years ago

@mkArtakMSFT Thanks for the update with regards this, IMHO this feature has a compelling use case, would the team consider tagging as an UPFORGRABS with a community PR along with input from @davidfowl or guidance on how he sees the implementation?

SteveSandersonMS commented 4 years ago

@grahamehorner It's more of a design question. There's no difficulty in implementing it. The work and the cost is all about figuring out what impact this would have, what the tradeoffs are, if there are better alternatives, and so on.

grahamehorner commented 4 years ago

@SteveSandersonMS oops; sorry Steve got my GitHub windows mixed up; and replied to the wrong issue/feature request in error :D BTW nice work on #Blazor WASM looking forward to using this in earnest for a few projects now have in the pipeline :D

markzielinski commented 4 years ago

It would be really nice to have OnBeforeRender as there are many use cases for wanting to do work before the rendering actually takes place, just as there will be many use cases for wanting to do work after the rendering takes place. As there is already a OnAfterRender it makes complete sense here to have a OnBeforeRender for the exact same reasons and to provide symmetry. Adding one lifecycle method isn't suddenly going to turn into a whole bunch of lifecycle methods added.

This was originally discussed two years ago and at that time there was a pull request submitted for this functionality, but it was not accepted in favor of making StateHasChanged virtual as discussed here. As there hasn't been progress made on this in those two years because of the complexity of determining what impacts this would have and what the tradeoffs are, this tells me that this was the wrong decision and the original request and solution should be revisited as it's the simplest solution and shouldn't cause any impacts.

bitbound commented 3 years ago

I found this thread while looking for OnBeforeRender event.

I agree with @markzielinski that the existence of OnAfterRender makes it 100% sensible to have OnBeforeRender.

alexhorner commented 2 years ago

Have a bump. Found a usecase for an OnBeforeRenderAsync or a StateHasChangedAsync.

I'd like to implement transitions into my application between pages. In order to achieve this, I need an OnBeforeRenderAsync or StateHasChangedAsync to activate the transition, wait for the transition to complete, render the new content then start a transition back.

The BlazorTransitionableRoute library doesn't meet my needs as it forces the new and old contents of the component to be rendered simultaneously.

I essentially need to achieve a curtain effect. The user clicks a link, a curtain transition starts to hide the existing content, the new content is swapped in behind the curtain, then the curtain is opened again to reveal the new content.

The curtain closure would be triggered by an OnBeforeRenderAsync or StateHasChangedAsync which will wait for the curtain transition to complete before allowing the new content to be rendered. The new content will be rendered, and then OnAfterRenderAsync will be called, which will trigger the curtain to open again.

kaleidocore commented 2 years ago

We absolutely need OnBeforeRender/OnBeforeRenderAsync. We're building control libraries and currently there is NO GOOD PLACE to hook in and do pre-render work in our base classes.

The current Blazor Lifetime Events suck in non-intuitive ways. Many of our base components fetch data from external sources and need to refresh on their own, which also requires pre-rendering work and initialization. The only way to do this now is to send out written memos to every developer who sub-classes the base components, asking them to PLEASE call method A,B and C in the top of their Razor code. And this still doesn't even give us an async path.

The only conceivable way to do async pre-rendering is to keep a manual bool DoNotRender flag, make sure all sub-classes look at this flag at the top of their .razor and skip rendering if set, override OnAfterRenderAsync in the base, check DoNotRender flag and if it's set perform the so-called "pre-render" work, turn off the flag, call StateHasChanged again to finally render, and make sure to set the flag again before any subsequent renders. That's a dumpster fire right there.

I think #15456 should be re-opened as even making StateHasChanged virtual wouldn't adequately solve the problem. We need to do pre-render work, but there's no need to do pre-render work if ShouldRender prevents actual rendering. And I have no desire to duplicate the internal logic of StateHasChanged and evaluate ShouldRender in a (hopefully) compatible way.

Adding OnBeforeRender/OnBeforeRenderAsync just before BuildRenderTree would be both intuitive, symmetrical, and would solve our problems nicely without clutter.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 1 year ago

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.