avoid the infinite loop issue of calling AddModuleMessage method.

[zyhfish]

No description provided.

[sbwalker]

@zyhfish this PR is actually not going to work because ModuleMessage is used in a variety of scenarios - not always within a module (ie. it is used in the SiteRouter, Themes, etc...). So the addition of the extra parameters and the fact that the caller needs to pass those parameters means that this solution is a breaking change.

[zyhfish]

Thanks @sbwalker , I updated the Visible parameter's default value to true so that it will not break any existing references.

[sbwalker]

@zyhfish much better :) Basically, it appears you moved the StateHasChanged() logic from ModuleBase to RenderModeBoundary - which prevent the infinite looping issue and still allows a message to be displayed in OnParamatersSet within module components. I will merge this now and you can submit a PR to re-introduce the AddModuleMessage() calls I removed earlier this week.

[sbwalker]

@zyhfish this PR seems to have some side effects. When I manage module settings and choose the Permission tab, an extra close icon is displayed - which I believe is coming from the ModuleMessage component - however if the message is blank then the close button should not be displayed - in fact the component should not render anything in this scenario.

Basically this is related to what I was referring to in my earlier comment - the ModuleMessage component can be used as a standalone component - and in those cases if the message parameter is blank, the component should not render any markup.

[sbwalker]

@zyhfish please verify if #3906 resolves the issue I outlined above

[zyhfish]

Hi @sbwalker , yeah the fix works great, thx so much!

[sbwalker]

@zyhfish I am not sure if the new Visible parameter is even necessary? It seems that the Message parameter can be used to determine if a ModuleMessage component needs to render markup. I logged another issue #3907 which I am not sure is related to the recent changes or not.

[zyhfish]

Hi @sbwalker , the Visible parameter is needed in the RenderModeBoundary control, as there have 2 references, which is at top, another is at bottom, only one are visible and decided by the position value.

[sbwalker]

@zyhfish I still do not think its necessary. RenderModeBoundary has a reference to 2 different instances of the ModuleMessage component. It is possible to call RefreshMessage on either of them independently (which basically calls StateHasChanged to update the UI). So there is no need for a Visible property - the Message property value can be used to determine visibility.

[zyhfish]

Hi @sbwalker , let me try to check whether it can works. I also tried to verify #3907 by reverting the changes of this, the error still occur, it maybe related to the dynamic render component in RenderModeBoundary that some component was not render in the refresh method, but need more investigation to identity the root cause.

[zyhfish]

HI @sbwalker , I submitted PR #3910 to remove the Visible parameter, please help to review, thx.

[sbwalker]

@zyhfish I never noticed this previously... but this PR now requires 2 ModuleMessage components to be created for each RenderModeBoundary component instance - whereas the previous implementation only created ModuleMessage components dynamically when they were required. From a performance and scalability perspective the new approach is not optimal, as the goal should always be to minimize the number of components required at run-time (since every component consumes state and needs to process life cycle events). So although this PR resolved the infinite loop issue when AddModuleMessage was called in OnParametersSet, it introduces other problems in terms of overhead. I think we are going to need to revisit this item to try and find a better solution.

[zyhfish]

This will be a challenge that if need to render the component dynamically then we need to trigger render method of RenderModeBoundary component instance, which is the root cause of the infinitely loop issue. let me try to check whether can find a magic solution to cover this.

[zyhfish]

Hi @sbwalker , I submitted PR #4268 to fix the issue by only re-render the components when the message really changed, please check whether it's better now.