-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Components] Prerrendering startup experience #7770
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
[Components] Prerrendering startup experience #7770
Conversation
1dc83a2
to
d5d8ea3
Compare
src/Mvc/Mvc.ViewFeatures/src/Microsoft.AspNetCore.Mvc.ViewFeatures.csproj
Outdated
Show resolved
Hide resolved
d5d8ea3
to
4f97821
Compare
src/Components/Server/src/Builder/RazorComponentsApplicationBuilderExtensions.cs
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilder.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilder.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
public IServiceProvider Services { get; } | ||
|
||
public Task<IEnumerable<string>> PrerrenderComponentAsync(Type componentType, ParameterCollection parameters) |
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 still don't love that this is IEnumerable<string>
but it seem OK for now.
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.
@rynowak For the MVC case we need to buffer the output as we need to write it synchronously. We could buffer it on a byte array, but I believe that would be more painful.
Another option would be to abstract it behind a type and provide overloads to write it to a stream/pipe (essentially an IHtmlContentResult like type).
src/Components/Server/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/DependencyInjection/ComponentDescriptor.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/DependencyInjection/ComponentDescriptor.cs
Outdated
Show resolved
Hide resolved
src/Components/Server/src/DependencyInjection/ConfigureStaticFilesOptions.cs
Show resolved
Hide resolved
var prepareResponse = options.OnPrepareResponse; | ||
if (prepareResponse == null) | ||
{ | ||
options.OnPrepareResponse = BlazorApplicationBuilderExtensions.SetCacheHeaders; |
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.
hmmm
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 is one of the first things I've seen (based on this approach) which makes me wonder if it's not totally generic.
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.
IMO, it's totally generic, but IMO in general it's easy for us to get it right, but easy for users to get wrong. It also makes it really hard to reorganize individual callbacks,etc in order to resolve conflicts (what if you have two pieces that add a callback and override each other).
In general, when we have this type of composition, I like it when it's backed by primitives and visible, so that:
- I get a clear understanding of how the system works.
- I can reorder/reconfigure the system to suit my needs without having to go bonkers or digging into the details of how each component on the stack works.
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 we extend the static files system to have a more native mechanism for plugging in multiple content sources, perhaps it also needs to be able to have things like response header defaults on a per-content-source basis.
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.
Yeah Steve found a better way of saying what I was thinking. It's a bit of a heavy hand for us configure caching settings for all static files, yet we do care about having this set for our stuff.
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 think it's OK to have their quirk in this release, but we should known issues or release notes this.
66a3e48
to
7a6f848
Compare
32f17dc
to
96f9a13
Compare
src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs
Show resolved
Hide resolved
src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs
Show resolved
Hide resolved
src/Components/Server/src/DependencyInjection/ConfigureStaticFilesOptions.cs
Show resolved
Hide resolved
src/Mvc/Mvc.ViewFeatures/src/RazorComponents/MvcRazorComponentPrerenderer.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.ViewFeatures/src/RazorComponents/MvcRazorComponentPrerenderer.cs
Show resolved
Hide resolved
src/Mvc/Mvc.ViewFeatures/src/RazorComponents/Prerendering/IComponentPrerenderer.cs
Show resolved
Hide resolved
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 think there's a couple of design questions we need to resolve, but this PR looks great for preview3
src/Mvc/Mvc.ViewFeatures/src/RazorComponents/Prerendering/IComponentPrerenderer.cs
Show resolved
Hide resolved
src/Components/Server/src/Builder/EndpointBuilderComponentExtensions.cs
Outdated
Show resolved
Hide resolved
/// <param name="builder">The <see cref="IEndpointConventionBuilder"/>.</param> | ||
/// <param name="selector">A CSS selector that identifies the DOM element into which the <typeparamref name="TComponent"/> will be placed.</param> | ||
/// <returns>The <paramref name="builder"/>.</returns> | ||
public static IEndpointConventionBuilder AddComponent<TComponent>(this IEndpointConventionBuilder builder, string selector) |
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.
Should builder
be a strong type instead of the interface? This will offer these suggestions on all of the builders.
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.
Yeah, but that has its drawbacks too. If we wrap the builder then other conventions can't reliably downcast it to RouteEndpointConventionBuilder and you can't do things based on Order or Pattern.
I think its fine for now to leave it as it is. I have an email thread where I lay out the issues and discuss the pros/cons. I can file an issue to track it for preview4
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.
It looks like this might be your solution to manually registering signalR. I think it's a fine choice to leave this as-is for now 👍
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.
Yeah.
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.
It looks like this might be your solution to manually registering signalR. I think it's a fine choice to leave this as-is for now 👍
That too, but I'm just too tired to pull thoughts out of my head 😄
src/Components/Server/src/DependencyInjection/ConfigureStaticFilesOptions.cs
Outdated
Show resolved
Hide resolved
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.
from me! I'm really excited about the final shape of this!
19d88f0
to
9524dd6
Compare
Ubuntu has a failing unrelated test: Microsoft.DotNet.Watcher.Tools.FunctionalTests.DotNetWatcherTests.RunsWithIterationEnvVariable |
Time to disable that test, it fails EVERYWHERE |
The only thing failing here is that one test in Ubuntu. @mkArtakMSFT please override. Squash and merge with the following message
|
I just wanted to say, thank you for your hard work! |
app.UseRouting(builder => | ||
{ | ||
builder.MapRazorPages(); | ||
builder.MapComponentHub<App.App>("app"); |
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 looks so good now! Awesome job, @javiercn!
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.
It does. It’s also fast to load because of the pre-rendering
Runtime changes to support the new components startup experience.
I still need to do a bit of cleanup and add more tests, but the main pieces and functional tests are in to show that it works.
I'll expand the PR tomorrow morning with the changes to the components template.