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

Implement delayed execution in BaseAfterRenderComponent #4331

Merged
merged 7 commits into from
Nov 27, 2022

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Nov 25, 2022

Test for #4075

This test can be run only on Blazorise.Demo.Bootstrap.Server because it only has the controller with the endpoint weatherforecast/getforecast".

@using Flurl;
@using Flurl.Http;
@using System.Text.Json.Serialization;

<Row>
    <Column>
        <Field>
            <NumericPicker Decimals="0" SelectAllOnFocus="true" @bind-Value="@Forecast.TemperatureC" />
        </Field>
    </Column>
</Row>


@code {
    [Inject] NavigationManager NavMgr { get; set; }

    private WeatherForecast Forecast = new();

    protected override async Task OnAfterRenderAsync( bool firstRender )
    {
        await base.OnAfterRenderAsync( firstRender );

        if ( firstRender )
        {
            Forecast = await NavMgr.BaseUri.AppendPathSegment( "weatherforecast/getforecast" ).GetJsonAsync<WeatherForecast>();
            await InvokeAsync( StateHasChanged );
        }
    }
}

Test for #4114

<Row>
    <Column>
        <RichTextEdit @ref="editor" Theme="RichTextEditTheme.Snow" />
    </Column>
</Row>

@code {
    private RichTextEdit editor;

    private string html = "Hello world.";
    private bool isSet;

    protected override async Task OnAfterRenderAsync( bool firstRender )
    {
        if ( !isSet && editor != null )
        {
            isSet = true;

            await editor.SetHtmlAsync( html );
            StateHasChanged();
        }

        await base.OnAfterRenderAsync( firstRender );
    }
}

@David-Moreira
Copy link
Contributor

Am I missing something, isn't this basically what we had previously? PushDelayedExecuteAfterRender will never get called since ShouldDelayExecution is always false.
Everything works as it was working before, no?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2022

Am I missing something, isn't this basically what we had previously? PushDelayedExecuteAfterRender will never get called since ShouldDelayExecution is always false.

Almost, yes. ShouldDelayExecution is the only thing that separates it, but without it, it doesn't work as expected. At least I didn't find a way still.

@David-Moreira
Copy link
Contributor

Almost, yes. ShouldDelayExecution is the only thing that separates it, but without it, it doesn't work as expected. At least I didn't find a way still.

I don't follow. Unless I'm missing something this WILL NEVER be called
image
this WILL NEVER be true
image

So basically the code is redudant and could be removed. Again, Am I not seeing something clearly?

@David-Moreira
Copy link
Contributor

Ah, I was missing some projects to be loaded and wasn't finding references, but only RichTextEdit uses this??
image

Didn't NUmericPicker and Markdown had issues related to his?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2022

Are you sure you're not looking at the wrong commit again? The ShouldDelayExecution is explicitly overridden by the components that must behave a certain way. In this vase, RTE, Markdown, and NumericPicker.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2022

Haha ignore my last message. I didn't push all :D

@David-Moreira
Copy link
Contributor

Should Rendered = true; be set before PushDelayedExecuteAfterRender(). It's a very tiny window, but would it make sense?
image

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2022

Should Rendered = true; be set before PushDelayedExecuteAfterRender(). It's a very tiny window, but would it make sense? image

I've just now hit that tiny window. So, yes.

@David-Moreira
Copy link
Contributor

Other than that, I'm not sure how to reliabily reproduce the previous bugs, but everything seems to be working fine, so looks good. And Implemention got way cleaner.

@David-Moreira
Copy link
Contributor

Now another question is.. Why would this not be on "globally" by default? For "being safe" purposes?
Since if this would happen in any other component I guess it would be good to have this behaviour? or not?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2022

Now another question is.. Why would this not be on "globally" by default? For "being safe" purposes? Since if this would happen in any other component I guess it would be good to have this behaviour? or not?

What do you mean have it globally? You mean to always do that?

@David-Moreira
Copy link
Contributor

Now another question is.. Why would this not be on "globally" by default? For "being safe" purposes? Since if this would happen in any other component I guess it would be good to have this behaviour? or not?

What do you mean have it globally? You mean to always do that?

Yes that.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2022

@David-Moreira PR description edited to include test code. Cleaned the lifecycle to not depend on ShouldDelayExecution, but it still requires delayedExecuteAfterRenderQueue.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2022

PS. as long as we use override OnFirstAfterRenderAsync to initialize components I think we're safe with this approach.

@David-Moreira
Copy link
Contributor

PS. as long as we use override OnFirstAfterRenderAsync to initialize components I think we're safe with this approach.

So there's a chance we might miss use the components initialize? How would we break it then?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2022

PS. as long as we use override OnFirstAfterRenderAsync to initialize components I think we're safe with this approach.

So there's a chance we might miss use the components initialize? How would we break it then?

I went and double-check. Even if we don't use OnFirstAfterRenderAsync and just override OnAfterRenderAsync directly it will work. The only way that it doesn't work is if we do await base.OnAfterRenderAsync( firstRender ); before initialization.

THIS WILL NOT WORK

protected override async Task OnAfterRenderAsync( bool firstRender )
{
    await base.OnAfterRenderAsync( firstRender );

    if ( firstRender )
    {
        // try initializing here
    }
}

THIS WILL WORK

protected override async Task OnAfterRenderAsync( bool firstRender )
{
    if ( firstRender )
    {
        // try initializing here
    }

    await base.OnAfterRenderAsync( firstRender );
}

which makes sense 100%. So we're good to go. We can make it a good convention to use OnFirstAfterRenderAsync but even if we don't, it will still work.

@David-Moreira
Copy link
Contributor

Did you commit everything, what's the weatherforecast controller for?

@David-Moreira
Copy link
Contributor

LGTM

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 27, 2022

Did you commit everything, what's the weatherforecast controller for?

Just a thing for us if we ever need to test components with connections to the api. In its own doesn't do much but I don't want to recreate it every time. Other than that we can consider having more use cases in the demo app where rest api is used.

@stsrki stsrki added this to the 1.1 Support milestone Nov 27, 2022
@stsrki stsrki mentioned this pull request Nov 27, 2022
@stsrki stsrki merged commit 8a605f5 into rel-1.1 Nov 27, 2022
@stsrki stsrki deleted the rel-1.1-optimize-execute-methods branch November 27, 2022 11:38
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants