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

Blazor: Make ElementReference a class so it can be passed as a parameter #16821

Closed
mrpmorris opened this issue Nov 4, 2019 · 10 comments
Closed
Labels
area-blazor Includes: Blazor, Razor Components question

Comments

@mrpmorris
Copy link

mrpmorris commented Nov 4, 2019

Describe the bug

Because ElementReference is a struct and not a class it is not possible to pass one as a [Parameter] to another component.

If it were changed to a class with an internal setter for the ID then consuming components would automatically have a valid reference when accessing from their OnAfterRender* methods.

Changing it to a class would also give the ability to have an IsValid property that can be set to true/false whenever the @ref declaring component is created or destroyed. Consuming components (and the parent component itself) would have to check IsValid before performing any operations on the reference - this check could even be added to JSInterop when passing the ElementReference as a parameter.

####Current process
Step1: ElementReference has a value as it is a value struct, but its ID is uninitialized
Step2: During BuildRenderTree the ElementReference member is copied by value into the child component
Step3: HTML is rendered
Step4: Blazor replaces the ElementReference member with a valid value
Step5: OnAfterRender* is executed
Step6: The child component works with its copy of the original ElementReference member with its invalid ID

####Proposed process
Step1: During the BuildRenderTree ensure the ElementReference referenced by @ref has an instance.
Step2: ElementReference instance is passed to child component
Step3: HTML is renfered
Step4: Blazor updates the ID of the ElementReference member
Step5: OnAfterRender* is executed
Step6: The child component works on the original ElementReference member with its updated ID

For this to work in the most recent release we have to define our parameter as Func<ElementReference>, but this is not as intuitive - passing ElementReference directly will result in the child component holding on to an uninitialized identity even at the point the elements have all rendered as HTML.

To Reproduce

Index.razor

@page "/"

<input @ref=MyInput/>
<SomeChildComponent Control=MyInput/>

@code {
  ElementReference MyInput;
}

SomeChildComponent.razor

@inject IJSRuntime JSRuntime
@code {
  [Parameter] ElementReference Control;

  protected override async Task OnAfterRenderAsync(bool firstRender)
  {
    await JSRuntime.InvokeVoidAsync("Whatever", Control);
  }
}
@mrpmorris mrpmorris changed the title Blazor: Make ElementReference a class so it can be passes as a parameter Blazor: Make ElementReference a class so it can be passed as a parameter Nov 4, 2019
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Nov 4, 2019
@javiercn
Copy link
Member

javiercn commented Nov 5, 2019

@mrpmorris Thanks for contacting us.

We are unlikely to do that, as holding on to an ElementReference from a separate component is not safe, as components render independently.

Instead I recommend you follow a pattern similar to the one showcased in the example below.

ElementReferenceCommunicationSample.zip

@javiercn
Copy link
Member

javiercn commented Nov 5, 2019

Filed dotnet/AspNetCore.Docs#15517 to track updating the docs to cover this.

@mkArtakMSFT
Copy link
Member

Thanks for contacting us. We believe that the question you've raised have been answered. If you still feel a need to continue the discussion, feel free to reopen it and add your comments.

@mrpmorris
Copy link
Author

mrpmorris commented Nov 5, 2019

@javiercn I appreciate the effort you went to to answer this, thanks.

I don't think the approach you suggest is workable as it places requirements on the parent. Take this component as an example - https://github.com/mrpmorris/blazor-university/blob/master/src/JavaScriptInterop/HtmlElementReferences/Shared/AutoFocus.razor

All it does is to set focus to the control specified whenever it is created and then first rendered. This component should sit along-side the control it sets focus to and share its lifetime. The parent should have no part in controlling this functionality at all, especially not that much.

An ElementReference holds onto invalid references even within the component itself....

@page "/"

@if (ShowIt)
{
    <h1 @ref=H1>Hello, world!</h1>
}
<button @onclick=Toggle>Toggle</button>

<Button @onclick=ShowStatus>Show status</Button>

@code {
    bool ShowIt = true;

    ElementReference H1;

    private void Toggle()
    {
        ShowIt = !ShowIt;
        StateHasChanged();
    }

    private void ShowStatus()
    {
        System.Diagnostics.Debug.WriteLine(System.Text.Json.JsonSerializer.Serialize(H1));
    }
}

Note that if you click Toggle to hide the H1 and then click Show Status, you'll see the ElementReference holds an invalid reference.

If we were to make it a class we could easily have a boolean IsValid property that is set to false when the component is destroyed. That would remove any danger because it would be up to the consumer to check its reference is valid before doing anything with it.

The current method of passing ElementReferences is unintuitive, your suggestion is too complex, and people will naturally try to pass element references - it's best to support it with the framework updating the reference value + marking it as valid/invalid.

Please could you re-open this for further consideration?

@mrpmorris
Copy link
Author

mrpmorris commented Nov 5, 2019

Thanks for contacting us. We believe that the question you've raised have been answered. If you still feel a need to continue the discussion, feel free to reopen it and add your comments.

@mkArtakMSFT I don't think it does. I am unable to reopen it, could you do that for me please?
PS: It's a change request, not a question :)

#16821 (comment)

@MarkStega
Copy link

@mkArtakMSFT I have to agree with @mrpmorris about the proposed ElementReferenceCommunication 'solution'. It is an over complicated solution that just feels like the wrong approach to the problem at hand. It is far simpler to have a class with a valid indicator as Peter suggests.

@simonziegler
Copy link

Hi, as a suggestion, if the reference has an IsValid too then making it a class will avoid having to repeat code in components. Thanks!

@StevenTCramer
Copy link
Contributor

@mkArtakMSFT @javiercn Could we reopen this?

@mrpmorris example shows how the suggested method will also keep invalid references which is what Javier calls "not safe". So can we consider other options?

@javiercn
Copy link
Member

javiercn commented Nov 8, 2019

We've discussed this a bit more within the team and we still think this is not something we are planning to do.

  • This is something developers can implement themselves if they have/to need/to.
    • Just pass a class and modify the class when the element changes if you want to follow that approach (not recommended as per the point below).
  • The current solution of passing a class/func to the child/sibling component is not good, as you don't get to run code when the reference changes and have to proactively check for updates, which is not feasible in most cases.
    • To give a practical example, if the element reference is from a sibling component and you need to react when the element reference provided in the sibling component changes, your component has no way of running code when that happens, and if my never have if your component itself doesn't ever re-render after the initial render. Same thing is applicable to a parent/child case, your parent re-rendering doesn't imply you re-render.
  • You can run code on the client side of things with invalid references, you only need to check for null/undefined in the receiving end of the call.
  • The current proposed change is a breaking change that also comes with a performance cost.
    • Ultimately we need to trade-off many concerns when building a framework and in this case, making element references a class would come with a cost, when there is a solution around it when you need it that is pay for play and that can be build once and re-used everywhere.
  • The provided solution, although a bit "complicated" can definitely be simplified and encapsulated into a separate class that can be reused a cross multiple components.
    • This is something that you can build for yourself one time and reuse across your app or build into a library that can be shared by all your apps or published to Nuget to enhance the Blazor experience.

I hope this helps clarify things.

@mrpmorris
Copy link
Author

Thanks for taking the time to discuss it!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components question
Projects
None yet
Development

No branches or pull requests

6 participants