MudComponentBase: Add explicit IMudStateHasChanged.StateHasChanged #6563
Conversation
ScarletKuro
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #6563 +/- ##
==========================================
- Coverage 91.47% 91.44% -0.03%
==========================================
Files 393 393
Lines 14877 14871 -6
==========================================
- Hits 13608 13599 -9
- Misses 1269 1272 +3
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
henon
changed the title
|
Awesome, thanks @ScarletKuro |
|
What about the other components, like MudTabs and etc. ? |
|
Don't they all inherit this? |
It is not public. |
|
They do @henon |
| @@ -45,5 +46,8 @@ public abstract class MudComponentBase : ComponentBase | |||
| /// If the UserAttributes contain an ID make it accessible for WCAG labelling of input fields | |||
| /// </summary> | |||
| public string FieldId => (UserAttributes?.ContainsKey("id") == true ? UserAttributes["id"].ToString() : $"mudinput-{Guid.NewGuid()}"); | |||
|
|
|||
| /// <inheritdoc /> | |||
| void IMudStateHasChanged.StateHasChanged() => StateHasChanged(); | |||
There was a problem hiding this comment.
This needs to be public, so it can be available.
There was a problem hiding this comment.
I see, that was actually my intention to make it publicly available. Probably even without the explicit interface (if the team agrees) because I am sure it will cause many questions which we'll have to answer over and over again because the method won't show up in intellisense without a cast to (IMudStateHasChanged).
There was a problem hiding this comment.
I got it, this may need to be documented because it is really confusing.
MudDialogInstance does not need to be casted for example.
I am trying to re render MudTabs and didn't know how to execute it until I red the PR description.
There was a problem hiding this comment.
This needs to be public, so it can be available.
It is "public". The IMudStateHasChange interface is public, and all components implement this interface. This means that you can cast to this interface and call StateHasChanged, as @henon suggested. This was the primary idea.
MudDialogInstance does not need to be casted for example.
We made the MudDialogInstance more special due to its unique requirement for the title refresh. We also anticipated that many people would find it confusing when ForceRerender is removed in v7, which is why we made this decision to make it more visible.
My main goal in implementing this interface in a "hidden" manner was to align with Microsoft's logic that StateHasChanged is protected and can't be called outside of the component. However, there are cases when there is an urgent need to call it, which is why we decided to make it possible through this interface.
While @henon believes that StateHasChanged is harmless, I disagree because overusing it can cause unintended side effects, such as input or state resetting / degrade of the performance because of too much re-renders. People will complain and create issues, then we will spend time on fixing users code mistake.
Nevertheless, people who understand the reasons behind using this method can find it through refactoring.
Ultimately, the decision to create this interface was made to balance the need for urgent updates with the potential risks of overusing StateHasChanged.
There was a problem hiding this comment.
I rather give people a shotgun and tell them "don't shoot yourself in the foot" than hiding the shotgund and having to explain every starving trapper where to find it ;)
I am preparing a PR to make it publicly visible with a comment of caution in the docs. We'll decide as a team whether or not to merge it.
There was a problem hiding this comment.
I see your argument and I partially agree. However, in this example the user could avoid the MudAutocomplete to refresh if they knew about
StateHasChanged()and refreshed other parts of the page selectively without affecting the autocomplete. So you see, it is a sharp knife, but if well documented and used correctly it can do a lot of good.
@henon I think you're misunderstanding the bug I opened. You're implying that I should call StateHasChanged more selectively, and it won't affect MudAutocomplete - but that isn't the case at all. The times I'm hitting the bug in my app are when I'm calling StateHasChanged very explicitly in a child control which is nowhere in/near the render tree of the MudAutoComplete control - but it's causing the autocomplete dropdown to dismiss regardless. This is a MudBlazor bug - for two reasons:
- It didn't used to happen like that - it was introduced in a MudBlazor release sometime in the last year or so
- You're effectively saying that no app anywhere should use
StateHasChangedwithin a control, in case it adversely affects MudBlazor's controls. Clearly that's bogus -StateHasChangedis part of the Blazor idiom, so calling it in my own scoped controls shouldn't cause MudBlazor to break. I'm basically looking at replacing the MudAutoComplete in my app because of this bug, which makes my app unusable.
There was a problem hiding this comment.
It didn't used to happen like that - it was introduced in a MudBlazor release sometime in the last year or so
Would it be difficult for you to locate the latest working version? This would greatly assist in identifying the commit responsible for the regression.
It seems highly likely that the encountered problem aligns with the one described in detail here: https://learn.microsoft.com/en-us/aspnet/core/blazor/components/overwriting-parameters?view=aspnetcore-7.0
A comprehensive overhaul of the MudAutocomplete component is necessary. However, I cannot provide an estimated time. It is likely that this will be addressed in a future major release(v7), but for now the core team is fully occupied with other tasks. If someone is seeking a swift resolution to the issue, taking the initiative to create a pull request may be the most expedient approach since MudBlazor is a community-driven project.
There was a problem hiding this comment.
Would it be difficult for you to locate the latest working version? This would greatly assist in identifying the commit responsible for the regression.
I tried to narrow it down, but there are just too many versions, and it takes a few minutes to repro each one.
Also, understand the team is busy, but it would be good to get this fixed at some point!
There was a problem hiding this comment.
@Webreaper with git bisect you'll find the commit in half an hour. I did so before for other issues. It is quite easy, see: #4303 (comment)
There was a problem hiding this comment.
Here is another one where the user used bisect and found that the bug was in their code base: #3594 (comment)
henon
changed the title
|
Any update on the plans to refactor/overhaul the statehaschanged stuff, so that this (and my associated issue #6686) is fixed? |
This problem has nothing to do with this particular PR. The problem usually arises when you write directly to the parameters, such problem indeed exist in the core library. About plans, I personally do not plan, |
Description
We had an internal discussion about this in discord.
This adds
IMudStateHasChangedinterface with methodStateHasChanged.The interface is added to the
MudComponentBaseand implemented it explicitly!This means that in order to use
StateHasChangedyou need to cast to theIMudStateHasChangedinterface.I still think it should be this way. It shows a clear intention, if it was that cheap and safe then Microsoft wouldn't do a breaking change by removing the public visibility for
StateHasChanged.In the end, if the user will need to call
StateHasChangedfor our components, he is free to use the interface as well.Types of changes
Checklist:
dev).