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

Static pages amid global interactivity #54948

Closed

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Apr 4, 2024

Fixes #51046 following the design approach discussed (and I think agreed on).

The only meaningful design change since the prior discussion is:

  • Previously we talked about a new attribute, so you'd do @attribute [StaticPage]
  • I realised we could make it cleaner by doing it as a parameter on RouteAttribute instead, so it's impossible to talk about static-ness for anything other than page components. This evades an entire class of possible confusion/misuse.
  • However this does also necessitate implementing the Razor syntax (e.g., @staticpage) so I will do that as well

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Apr 4, 2024
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/static-pages-amid-global-interactivity branch from 7c6ab99 to ab01b76 Compare April 5, 2024 08:57
? null
: RenderMode.InteractiveAuto;
: RenderMode.InteractiveWebAssembly;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this from Auto to WebAssembly to make the E2E tests more deterministic, and because WebAssembly is the more complicated case anyway.

<a class="dismiss">🗙</a>
</div>

<Routes />
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for changing Samples/BlazorUnitedApp like this is just to bring it more into alignment with conventional projects.

What we had here before was a pretty weird point-in-time snapshot from the early .NET 8 days and couldn't easily be switched between global and per-page interactivity, which is something you might often want to do when experimenting with different features.


var component = effectiveRenderMode is null
? _componentActivator.CreateInstance(componentType)
: _renderer.ResolveComponentForRenderMode(componentType, parentComponentId, _componentActivator, effectiveRenderMode);
Copy link
Member Author

Choose a reason for hiding this comment

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

Conceptually the change here is factoring out the "resolve rendermode" logic into a new virtual method on the renderer, so that we can implement the new rule about static pages suppressing callsite rendermodes. Doing so happens to simplify ComponentFactory quite a bit.

}
return Create(templatesByHandler, serviceProvider);
}

private static string[] GetTemplates(Type componentType)
private static IReadOnlyList<string> GetTemplates(Type componentType, bool includeStaticRoutes)
Copy link
Member Author

Choose a reason for hiding this comment

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

Replacing string[] with IReadOnlyList<string> is a possibly-excessive optimization so that we can just build the list in a single pass and don't have to then .ToArray() on it. TBH I could have skipped this change but it's not really difficult and I don't think there's any drawback now it's done.

/// <summary>
/// Gets a flag indicating whether the page's route is declared as static.
/// </summary>
public bool IsStaticRoute { get; }
Copy link
Member Author

Choose a reason for hiding this comment

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

This could go into an entirely separate metadata class but then that's an extra metadata entry we have to look up on every request, and I don't see there being much reason for it.

if (componentTypeMetadata.IsStaticRoute)
{
_renderer.SuppressRootComponentRenderModes();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bit where we take the endpoint metadata and turn it into a behavior on the renderer.

return base.ResolveEffectiveRenderMode(componentType, parentComponentId, componentTypeRenderMode, callerSpecifiedRenderMode);

bool IsRootComponent(int? componentId)
=> !componentId.HasValue || GetComponentState(componentId.Value).ParentComponentState is null;
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 5, 2024

Choose a reason for hiding this comment

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

Most of the changes here are because, after making ResolveEffectiveRenderMode as a new virtual method, it makes sense to do more of the decision-making about rendermode boundaries at this stage instead of later in ResolveComponentForRenderMode, as it simplifies the logic and avoids some duplications.

It's an internal sealed type so this can't be breaking.

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review April 5, 2024 12:16
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner April 5, 2024 12:16
Comment on lines +33 to +37
if (_suppressRootComponentRenderModes && IsRootComponent(parentComponentId))
{
// This component is the start of a subtree with a rendermode, so introduce a new rendermode boundary here
return new SSRRenderModeBoundary(_httpContext, componentType, renderMode);
// Disregard the callsite rendermode because this is in the root component (or is the root component itelf)
// and we've been configured to suppress render modes for the root component.
callerSpecifiedRenderMode = null;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this logic disabling interactivity for all root components? Won't this mean that if there is an interactive component elsewhere in your app component it won't get interactivity?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 5, 2024

Choose a reason for hiding this comment

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

Yes, that's exactly what this is meant to do.

// This match will be the top-level InteractiveGreetingWebAssembly
// The fact that interactivity is *not* suppressed here shows that SuppressRootComponentRenderModes
// only suppresses *call-site* rendermodes in the root. It does not stop a child from being interactive
// if it has a @rendermode on the child component type itself.
Copy link
Member

Choose a reason for hiding this comment

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

I see that you discussed this in your last comment on the issue with this bit:

That is, we would define the meaning of @attribute [StaticPage] or @staticpage as follows:

"Suppresses any @rendermode directives in your root component"
..
I know in theory people could structure their site in more unexpected ways, for example having some combination of other components wrapped around their router, with @rendermode in one of them. In my opinion the high benefit of fixing the "all or nothing" problem is worth that, since there's still a simple and clear definition of "static page" that should be understandable.

This behavior is surprising to me, but maybe it's because of my relative lack of experience using Blazor.

I don't like the idea that refactoring my root component into subcomponents could just break my app in unexpected ways. It feels unsound to expect that everyone is going to structure their components the way our project templates do. If we give people the flexibility to arbitrarily nest the Router component, it shouldn't then break static routes when they do. Are there other ways that rendering the root component is already similarly special?

Should we now raise some sort of error if we detect a non-root-component with a @rendermode attribute that includes the Router component if there are any static routes? Or could we suppress any @rendermode that affects a Router parent whether or not it's in the root component when rendering a @staticpage?

It seems to me that whether or not the @rendermode affects the Router is more fundamental to making static routes work than whether or not the @rendermode exists in the root component.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 5, 2024

Choose a reason for hiding this comment

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

It feels unsound to expect that everyone is going to structure their components the way our project templates do.

I get that and agree with it. The reason I'm OK with it here is that:

  1. People don't have to use this feature. If they want to control stuff in more custom ways they can do so by setting @rendermode=SomeExpression like the auth templates do, and pass that through in whatever way they want.
  2. It's a clear, logically coherent definition of the feature that it suppresses rendermodes in the root component, and this covers what people are asking for without requiring any app upgrade steps in typical cases.
  3. We have to have some way to distinguish which component instances you want to force to be static and which you don't (e.g., arbitrary interactive components embedded as children into your static pages), and we want to achieve that without forcing extra configuration or upgrade steps.

Should we now raise some sort of error if we detect a non-root-component with a @rendermode attribute that includes the Router component if there are any static routes?

TBH I don't think that's realistic. It would be much too coupled to the Router component and we have no way to know whether a given use of Router is even involved in this pattern.

Or could we suppress any @rendermode that affects a Router parent whether or not it's in the root component when rendering a @staticpage?

I'm afraid we render top-down so we don't know what will end up in the descendant hierarchy - arbitrary code could include/exclude a Router component dynamically.

It seems to me that whether or not the @rendermode affects the Router is more fundamental to making static routes work than whether or not the @rendermode exists in the root component.

It's not only about the router. For example in the most common scenario it's also about HeadOutlet. And in this most common scenario it's not even the router's rendermode who changes - the default template sets the rendermode on an app-specific component called Routes.

I'm well aware this design targets a particular pattern, but they payoff from doing so is that there aren't any complicated rules. If our simple rule ("suppress rendermode in root component") works for your app then great, and if not, you have to find your own other solution. If there was a mechanism that correctly interpreted what kind of interactivity suppression patterns everyone would want in all cases, and it was trivial to explain and didn't require more code in people's apps, and was not highly expensive to implement, we'd certainly do that but keeping the feature simple and restrictive looks like the best way forwards to me.

Copy link
Member

@MackinnonBuck MackinnonBuck Apr 5, 2024

Choose a reason for hiding this comment

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

I tend to have a perspective similar to @halter73's on this one.

We have to have some way to distinguish which component instances you want to force to be static and which you don't (e.g., arbitrary interactive components embedded as children into your static pages), and we want to achieve that without forcing extra configuration or upgrade steps.

While the rule is simple and coherent, it's practically invisible to customers, which makes its implied limitations also invisible. Customers may perform a seemingly harmless refactor that breaks their app, and nothing in their code will suggest the reason why that happened. For the sake of comparison, "option 2" (or one of its alternatives) from your original proposal would make likely the mechanics of the feature much clearer, which IMO was one of the positive aspects of it, despite it requiring an extra upgrade step.

People don't have to use this feature

Agreed, although I'm not sure yet what results in maximizing net customer happiness. Does the joy people get from using this feature without any configuration offset the sadness that a smaller number of customers face when they discover that they've fallen off the cliff of not being able to use the feature anymore? I'd personally rather be the customer that has to follow some extra steps to enable the feature than the one who spends an hour trying to figure out down the road why my refactor broke things.

All that said, I haven't found any open-source examples of deciding the global render mode outside the root component, so maybe this hypothetical refactor actually wouldn't be that common. So while I think it would be clearer if @staticpage meant "ignore all render modes" or if we took a more explicit approach like option 2, I don't actually have any evidence that the current design will cause real confusion among customers.

So I'm a currently a bit torn on what the right direction is, but I trust @SteveSandersonMS's analysis on this and am happy with proceeding with the current design.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 5, 2024

Choose a reason for hiding this comment

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

Thanks for the extra thoughts. I recognize there's some scepticism from folks about the simplifications and constraints involved in this design, and if the team isn't happy with it then we can hold back. It's not desirable to push this through if the team isn't on board with it. I'm a bit uncertain whether people are just each having to go through their own processes to internalize all the tradeoffs, or if I'm just pushing in a direction people can't buy into.

At the same time I don't see other good end-to-end design suggestions that fit the real scenarios without transferring pain to app developers. Everything within the option 2 category, to me, is like saying "if app developers are willing to write a bunch more code and remember futher restrictions about rendermode combinations, they get more flexibility" - which is true, but we should be hearing the message quite clearly that people are not looking to do that.

Some possible tweaks or middle-grounds that could exist:

  • We could go back to the rule that "@staticpage means literally everything is static, i.e., disables rendermodes completely".
    • I'm not convinced that meets the goals here, since people are asking for a way to do per-page SSR within a globally-interactive site. And SSR normally entails the option to embed interactive components if you want. Nobody has said their goal is to block interactivity, but rather it's to do SSR-style pages selectively.
    • However if I'm missing reasons to think that's what people are actually aiming to do, please let me know!
  • We could find a name that more literally communicates what is being done here, e.g., @disablerootinteractivity or @forcestaticroot.
    • If we can find a better name than @staticpage that would be great. I'm definitely not fixated on the current name - it's passable at best.
    • Names that rely on three words together are a bit much, given the lowercase, no-separators naming convention. If we can find a one or two-word name, certainly better! But I'm open to whatever here.

@MackinnonBuck @halter73 @javiercn Please let me know what product direction feels right to you! Do bear in mind the delivery capacity constraints we have to work in, too.

Copy link
Member

Choose a reason for hiding this comment

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

We could find a name that more literally communicates what is being done here

Yeah, I think if we could find a name that clearly conveyed what this feature was doing, that might be enough to address some of the concerns I was expressing. I'm just not sure if a name exists that's both descriptive and short enough to conveniently type and read.

I generally still can't quite push myself over the edge of being comfy with the framework ignoring the @rendermode gesture in such a specific manner. But I'm totally willing to be outvoted if others can get on board with it.

Copy link
Member

Choose a reason for hiding this comment

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

Everything within the option 2 category, to me, is like saying "if app developers are willing to write a bunch more code and remember futher restrictions about rendermode combinations, they get more flexibility" - which is true, but we should be hearing the message quite clearly that people are not looking to do that.

My impression was that most of the confusion from the community came not from how render modes composed, but how static rendering differs from interactive rendering and what quirks come with that. It seems like we already have a decent experience around making the render mode rules visible (e.g., if you nest them in an invalid way, or use an unsupported one, you get an exception). In addition to the existing rules, I believe just one additional rule would help catch misuses of an "option 2"-like approach: If the router gets rendered with a render mode that differs from the cascading page render mode, give a warning (or error?) that explains that this could cause the app to misbehave. And the existing rules around invalid nesting of render modes would still apply.

The approach still obviously has the downside that it requires changes to application code, but at least the app code clearly has control over which components get rendered with what render mode. Maybe there would need to be more restrictions/rules that I'm not thinking of, so please let me know if I'm missing something!

I'd be curious to know what @halter73 and @javiercn think - if those two are also supportive of the current approach then I'll happily concede!

Copy link
Member

Choose a reason for hiding this comment

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

One other thing that just came to mind:

There might be a scenario where a developer wants a page to be routed statically, but there's another component in the top-level layout of the app that should always be interactive (e.g., a "chat" button/window that's always there regardless of how the current page was routed). I don't have a sense of how common it would be to place a component like that in App.razor instead of a layout component, but that's a case where debugging could be very confusing if the developer does not understand when render modes get ignored.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 15, 2024

Choose a reason for hiding this comment

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

Yeah, I think if we could find a name that clearly conveyed what this feature was doing, that might be enough to address some of the concerns I was expressing.

The most explicit but plausibly short name I can think of is @disablerootrendermodes.

The approach still obviously has the downside that it requires changes to application code

I personally think that any design that requires upgrade steps for existing projects, and complicates the default template for new projects, is undesirable. There's pretty strong signal from the community about that.

We're basically in a corner where it seems that we can't simultaneously satisfy all of these design goals:

  • [a] Allows individual pages to opt out of global interactivity
  • [b] Does not stop you from making other components interactive
  • [c] Does not require upgrade steps or new complications to the default template
  • [d] Is not specific to particular architectural patterns (e.g., the "global interactivity" pattern)

The design in this PR satisfies [a-c] but not [d]. Suggestions from others do satisify [d] but not [c].

I'm not sure it's possible to satisify [a-d] simultaneously in any design, since the "global interactivity" pattern in existing apps inherently specifies a rendermode for all pages at the root, and so opting out would innately mean overriding that in some sense (i.e., we have to know to disregard at least the root rendermode). Nothing in the "option 2" space solves this without forcing more code/conditions in some combination of Program.cs and App.razor, violating constraint [c].

There might be a scenario where a developer wants a page to be routed statically, but there's another component in the top-level layout of the app that should always be interactive (e.g., a "chat" ...)

That would generally be fine since you're only suppressing @rendermode= in the root component. The developer is still free to put a @rendermode directive on Chat.razor itself and that won't be suppressed. Or if the @rendermode= is in the layout (which is not the root component) then it won't be suppressed there either.

I do agree that the distinction between which rendermodes are suppressed and which aren't wouldn't be obvious at least with the original name. Renaming it to @disablerootrendermodes would act as a fairly strong reminder even if it still isn't 100% obvious at first glance.

public ComponentTypeMetadata([DynamicallyAccessedMembers(Component)] Type componentType, bool isStaticPage)
: this(componentType)
{
IsStaticRoute = isStaticPage;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We can discuss this in API review, but is there a particular reason for naming the parameter isStaticPage while the property is name IsStaticRoute?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good point. It should probably be IsStaticRoute all the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

And more broadly as @javiercn has already suggested, the whole name @staticpage could do with change/clarification. We want a name that means "suppress global interactivity for this page" but @suppressglobalinteractivity doesn't look great!

@SteveSandersonMS
Copy link
Member Author

Closing in favour of #55157

@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
4 participants