DisplayAsync to return actual pre-rendered IHtmlContent #15247

ns8482e · 2024-02-03T18:02:23Z

Component tag helper / Html Helper is not able to render Razor Components for nested PositionWrapper components in .NET8.

Suggested change here to have, DisplayAsync to return actual pre-rendered IHtmlContent instead of PositionWrapper

Fixes #15235
Fixes #15211

/cc @sebastienros

…sitionWrapper. Fixes #15235 Fixes #15211

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DisplayHelper.cs

src/OrchardCore/OrchardCore.DisplayManagement/PositionWrapper.cs

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DisplayHelper.cs

sebastienros · 2024-02-08T19:27:07Z

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs

@@ -50,6 +50,16 @@ public async Task<IHtmlContent> ExecuteAsync(DisplayContext context)
            // Check if the shape is pre-rendered.
            if (shape is IHtmlContent htmlContent)
            {
+                if (htmlContent is PositionWrapper wrapper)


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.

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

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.

sebastienros · 2024-02-09T17:45:44Z

there no need of nesting PositionWrapper in PositionWrapper.

I had not understood this at first look. Now I get it, I thought your code was removing all wrappers, not just the nested ones. And this is raising a good point actually, maybe we shouldn't wrap position wrappers, that is kind of a bug. Can you try to do that instead?

We could also add a static IHtmlContent TryWrap() in PositionWrapper to handle string, PositionWrapper and IHtmlContent (using pattern matching) so we don't duplicate the logic and it would return the correct results.

ns8482e · 2024-02-10T20:24:16Z

I'll update the code, to use static TryWrap instead of using constructor. However to avoid nested wrapper we still need to unwrap the wrapper to return underlying IHtmlContent on shape execute / DisplayAsync.

…er is now sealed and have private constructor.

ns8482e · 2024-02-10T21:57:31Z

maybe we shouldn't wrap position wrappers

FYI @sebastienros , this happens when the PositonWrapper is within the _items of ZoneShapes and DisplayAsync is called on Zone / ContentZone.

sebastienros · 2024-02-20T21:06:34Z

src/OrchardCore/OrchardCore.DisplayManagement/PositionWrapper.cs

+
+        public static PositionWrapper TryWrap(object value, string position)
+        {
+            if (value is PositionWrapper wrapper)


is IPositioned?

I used PositionWrapper because return value must be both IPositioned and IShape

sebastienros · 2024-02-20T21:07:43Z

src/OrchardCore/OrchardCore.DisplayManagement/Shapes/Shape.cs

@@ -65,13 +65,13 @@ public virtual ValueTask<IShape> AddAsync(object item, string position)

            _items ??= [];

-            if (item is IHtmlContent)
+            if (item is IHtmlContent htmlContent)


Can't this whole if/else/else be a single TryWrap?

sebastienros · 2024-02-20T21:09:06Z

src/OrchardCore/OrchardCore.DisplayManagement/PositionWrapper.cs

+            else if (value is string stringContent)
+                return new PositionWrapper(stringContent, position);
+            else
+                throw new System.NotSupportedException($"Type of {nameof(value)} must be either {nameof(String)}, {nameof(IHtmlContent)} or {nameof(PositionWrapper)}.");


TryWrap should it return a new wrapper in this case instead of throwing?

Should it unwrap if the position is "" ? Is that the meaning of "" in ThemeLayout.cs?

Should it unwrap if the position is "" ? Is that the meaning of "" in ThemeLayout.cs?

It's default, I believe when empty it will sort based on the order of items added

TryWrap should it return a new wrapper in this case instead of throwing?

Throwing NotSupportedException because value is not either string, IHtmlContent or PositionWrapper. Position wrapper doesn't support any other object - unless we return null instead of throwing exception - please suggest @sebastienros

Updated to return null instead of throwing exception

sebastienros · 2024-02-21T20:42:39Z

I saw a test failing on the CI

ns8482e · 2024-02-21T20:50:18Z

I saw a test failing on the CI

fixed

…5247) Fixes OrchardCMS#15235

DisplayAsync to return actual pre-rendered IHtmlContent instead of Po…

7d458a8

…sitionWrapper. Fixes #15235 Fixes #15211

ns8482e requested a review from sebastienros February 3, 2024 18:09

MikeAlhayek reviewed Feb 3, 2024

View reviewed changes

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs Outdated Show resolved Hide resolved

MikeAlhayek reviewed Feb 3, 2024

View reviewed changes

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DisplayHelper.cs Outdated Show resolved Hide resolved

MikeAlhayek reviewed Feb 3, 2024

View reviewed changes

src/OrchardCore/OrchardCore.DisplayManagement/PositionWrapper.cs Outdated Show resolved Hide resolved

ns8482e mentioned this pull request Feb 3, 2024

.net8+orchardcore blazorserver mode render component error #15235

Closed

ns8482e added 2 commits February 3, 2024 12:23

react to feedback

eb7a1f0

update property

6bfff18

MikeAlhayek reviewed Feb 3, 2024

View reviewed changes

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DefaultHtmlDisplay.cs Outdated Show resolved Hide resolved

remove cast

01f86e9

MikeAlhayek approved these changes Feb 3, 2024

View reviewed changes

MikeAlhayek added the needs triage label Feb 6, 2024

MikeAlhayek reviewed Feb 6, 2024

View reviewed changes

src/OrchardCore/OrchardCore.DisplayManagement/Implementation/DisplayHelper.cs Outdated Show resolved Hide resolved

Update DisplayHelper.cs

72b54e8

sebastienros reviewed Feb 8, 2024

View reviewed changes

Added static methods TryWrap/Unwrap in PositionWrapper. PositionWrapp…

6fb8f0e

…er is now sealed and have private constructor.

formatting

47ddfa1

ns8482e requested a review from sebastienros February 10, 2024 22:11

ns8482e mentioned this pull request Feb 15, 2024

POC Blazor Component #15068

Open

sebastienros reviewed Feb 20, 2024

View reviewed changes

ns8482e and others added 4 commits February 21, 2024 14:08

react to review comments

4f14544

Update PositionWrapper.cs

0393d53

Update Shape.cs

e42fac8

Merge branch 'main' into ns/#15235

82645d9

sebastienros enabled auto-merge (squash) February 21, 2024 20:15

changed PositionWrapper to IPositioned

b331411

sebastienros merged commit eb0511f into main Feb 21, 2024
5 checks passed

sebastienros deleted the ns/#15235 branch February 21, 2024 20:48

github-actions bot mentioned this pull request Mar 1, 2024

Monthly community metrics report for 2024-02-01..2024-02-29 #15430

Closed

urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024

DisplayAsync to return actual pre-rendered IHtmlContent (OrchardCMS#1…

1e00bb7

…5247) Fixes OrchardCMS#15235

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DisplayAsync to return actual pre-rendered IHtmlContent #15247

DisplayAsync to return actual pre-rendered IHtmlContent #15247

ns8482e commented Feb 3, 2024 •

edited

Loading

sebastienros Feb 8, 2024

sebastienros Feb 8, 2024

ns8482e Feb 9, 2024 •

edited

Loading

sebastienros commented Feb 9, 2024

ns8482e commented Feb 10, 2024 •

edited

Loading

ns8482e commented Feb 10, 2024

sebastienros Feb 20, 2024

ns8482e Feb 21, 2024

sebastienros Feb 20, 2024

ns8482e Feb 21, 2024

sebastienros Feb 20, 2024

sebastienros Feb 20, 2024

ns8482e Feb 21, 2024 •

edited

Loading

ns8482e Feb 21, 2024

sebastienros commented Feb 21, 2024

ns8482e commented Feb 21, 2024

DisplayAsync to return actual pre-rendered IHtmlContent #15247

DisplayAsync to return actual pre-rendered IHtmlContent #15247

Conversation

ns8482e commented Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns8482e Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

sebastienros commented Feb 9, 2024

ns8482e commented Feb 10, 2024 • edited Loading

ns8482e commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns8482e Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Feb 21, 2024

ns8482e commented Feb 21, 2024

ns8482e commented Feb 3, 2024 •

edited

Loading

ns8482e Feb 9, 2024 •

edited

Loading

ns8482e commented Feb 10, 2024 •

edited

Loading

ns8482e Feb 21, 2024 •

edited

Loading