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.

Sign up for GitHub

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

DisplayAsync to return actual pre-rendered IHtmlContent #15247

Merged
merged 12 commits into from
Feb 21, 2024
Merged

DisplayAsync to return actual pre-rendered IHtmlContent #15247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7d458a8
DisplayAsync to return actual pre-rendered IHtmlContent instead of Po…
ns8482e Feb 3, 2024
eb7a1f0
react to feedback
ns8482e Feb 3, 2024
6bfff18
update property
ns8482e Feb 3, 2024
01f86e9
remove cast
ns8482e Feb 3, 2024
72b54e8
Update DisplayHelper.cs
ns8482e Feb 8, 2024
6fb8f0e
Added static methods TryWrap/Unwrap in PositionWrapper. PositionWrapp…
ns8482e Feb 10, 2024
47ddfa1
formatting
ns8482e Feb 10, 2024
4f14544
react to review comments
ns8482e Feb 21, 2024
0393d53
Update PositionWrapper.cs
sebastienros Feb 21, 2024
e42fac8
Update Shape.cs
sebastienros Feb 21, 2024
82645d9
Merge branch 'main' into ns/#15235
sebastienros Feb 21, 2024
b331411
changed PositionWrapper to IPositioned
ns8482e Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,19 @@ public async Task<IHtmlContent> ExecuteAsync(DisplayContext context)
// Check if the shape is pre-rendered.
if (shape is IHtmlContent htmlContent)
{
return htmlContent;
if (htmlContent is PositionWrapper wrapper)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this would not break something in OC. Or the PositionWrapper would be useless.
Have you tried to check where this is called (outside of Blazor) to see what could go wrong?

Also, can you explain why this is fixing the exception? Technically this wrapper is invoking the same WriteTo as if you return the wrapped value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of things to try with this change: Widgets, FlowPart, BagPart, GraphQL rendering, layouts, custom placement.

Copy link
Contributor Author

@ns8482e ns8482e Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not break anything in OC, rather fixes Blazor component rendering.

PositionWrapper is nothing but holds pre-rendered IHtmlContent, and it can only contain one IHtmlContent. Hence there no need of nesting PositionWrapper in PositionWrapper.

Currently In ThemeLayout we use PositionWrapper to pre-render (and useful while maintaining position on zone) before we place on Layout. This allows to have lazy placed zones to be rendered on Layout but requires us to call DisplayAsync twice 8880 on zones, that unnecessary nests PositionWrapper in PositionWrapper.

This PR returns the original pre-rendered IHtmlContent when the shape provided in DisplayAsync is PositionWrapper and avoids nesting of PositionWrapper when called multiple times.

This allows thread that Writes to the stream is the same that owns the Dispatcher that generated IHtmlContent for blazor component.

{
while (wrapper.Value is PositionWrapper)
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
{
wrapper = (PositionWrapper)wrapper.Value;
}

return wrapper.Value;
}
else
hishamco marked this conversation as resolved.
Show resolved Hide resolved
{
return htmlContent;
}
}

var shapeMetadata = shape.Metadata;
Expand Down
15 changes: 14 additions & 1 deletion src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DisplayHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Html;
using Microsoft.CodeAnalysis.CSharp.Syntax;
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved

namespace OrchardCore.DisplayManagement.Implementation
{
Expand Down Expand Up @@ -75,7 +76,19 @@ public Task<IHtmlContent> ShapeExecuteAsync(IShape shape)
// Check if the shape is pre-rendered.
if (shape is IHtmlContent htmlContent)
{
return Task.FromResult(htmlContent);
if (shape is PositionWrapper wrapper)
{
while(wrapper.Value is PositionWrapper)
{
wrapper = (PositionWrapper) wrapper.Value;
}

return Task.FromResult(wrapper.Value);
}
else
{
hishamco marked this conversation as resolved.
Show resolved Hide resolved
return Task.FromResult(htmlContent);
}
}

var context = new DisplayContext
Expand Down
8 changes: 8 additions & 0 deletions src/OrchardCore/OrchardCore.DisplayManagement/PositionWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ public PositionWrapper(string value, string position)
Position = position;
}

internal IHtmlContent Value
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
{
get
{
return _value;
}
}

public void WriteTo(TextWriter writer, HtmlEncoder encoder)
{
_value.WriteTo(writer, encoder);
Expand Down
Loading