-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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] Add an API to describe the render mode (if any) a component is running in #55577
Conversation
91621a7
to
ac1ad1b
Compare
The approach here is great for finding out the current rendering platform, but doesn't address finding out the effective rendermode at least in the sense that I was proposing. For example, even when a component is running on the Suggestion: let's keep this PR simple as it is, and just eliminate the What do you think? |
Sounds good, let me think for 5 minutes if what you suggest is straightforward to do, and maybe I just go ahead and do it. If I see any trouble with it, I'll just remove it for now. |
On However there is quite a bit more to test, as there are more combinations (e.g., need to cover all the renderers, and each case where it's possible to set rendermodes). So I'll leave it with you whether you want to incorporate that into this PR or not! |
Not sure what you mean by this, I think the main change would be that EndpointHtmlRenderer walks the tree to find the rendermode, but all other renderers just provide static results as they do already (server does server, wasm does wasm, webview does null). The main difference is that endpoints returned null, and now we are saying it'll return Auto, WebAseembly or Server based on the render mode for the root component (figuring it out with the help of the renderer), am I getting this wrong? So, it's only about checking those non-interactive returns the render mode instead of null, isn't it? |
ac1ad1b
to
c2b67f9
Compare
By "a bit more to test" I just mean writing E2E tests in the PR, not things getting tested at runtime :)
Yes exactly. |
Cool, check if c2b67f9 matches your expectations and I'll add more tests. |
Yay! Other than minor details yes that looks great. |
/// <summary> | ||
/// Gets the <see cref="IComponentRenderMode"/> the component is rendering or will render in when it becomes interactive. | ||
/// </summary> | ||
protected IComponentRenderMode? InteractiveRenderMode => _interactiveRenderMode ??= _renderHandle.GetInteractiveRenderMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just call it RenderMode? The return value is valid even if the component isn't interactive, plus we could add other noninteractive rendermodes in the future.
Also it would be good to change the caching rule so it caches even if the value is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I wanted to add interactive in there is because I'm worried of people writing
if(RenderMode is InteractiveServer)
{
`JSRuntime.InvokeAsync(...)`
}
So I wanted to give it a name that gave a hint as to what this meant/covered and didn't want to call it NonPrerendered render mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If people try to use JS interop when it's not available, it will just fail telling them it's not available, so TBH I don't think there's a problem with this. The correct code would be "if (Platform is server) { ... }" which is similarly easy to write so I think people will be OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, let's go with RenderMode then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do GetRenderMode() instead because RenderMode is a type and that will cause all sort of symbol resolution issues
c2b67f9
to
1017c33
Compare
🆙 📅 |
Applied API review feedback |
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyRenderer.cs
Outdated
Show resolved
Hide resolved
? (GridRendering.RenderMode)Enum.Parse(typeof(GridRendering.RenderMode), _gridTypeOption.Value(), true) | ||
: GridRendering.RenderMode.FastGrid; | ||
? (GridRendering.GridRenderMode)Enum.Parse(typeof(GridRendering.GridRenderMode), _gridTypeOption.Value(), true) | ||
: GridRendering.GridRenderMode.FastGrid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this shows it's a breaking change in a super niche sense, but we have to accept that otherwise we couldn't extend the base class at all. If there's a lot of pushback in previews we can consider options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we decided to go with AssignedRenderMode
instead of just RenderMode
for the property, hopefully it will be less breaking.
src/Components/test/E2ETest/Tests/InteractiveHostRendermodeTest.cs
Outdated
Show resolved
Hide resolved
...mponents/test/testassets/Components.TestServer/RazorComponents/Pages/ComponentPlatform.razor
Outdated
Show resolved
Hide resolved
Co-authored-by: Mackinnon Buck <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@@ -48,16 +48,14 @@ public ComponentBase() | |||
protected ComponentPlatform Platform => _renderHandle.Platform; | |||
|
|||
/// <summary> | |||
/// Gets the target <see cref="IComponentRenderMode"/> this component will run in after prerendering. | |||
/// Gets the <see cref="IComponentRenderMode"/> assigned to this component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is to avoid implying we are currently prerendering, or that prerendering is even applicable (since on WebView or standalone WebAssembly it wouldn't be applicable as a concept).
Adds an API to describe the current host render mode if any a component is running in.
Fixes #49401
Sample usage:
Adds a new
ComponentPlatform
type that the renderer provides (gets exposed through RenderHandle) to provide details on the platform the component is currently running on.All 4 renderers provide an implementation, and they indicate the name, whether the platform is interactive, and any render mode associated with the platform (if any).