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

Fixed bug - Passing options to UseStaticFiles breaks Blazor Server #45897

Merged
merged 4 commits into from Jan 9, 2023

Conversation

surayya-MS
Copy link
Member

@surayya-MS surayya-MS commented Jan 5, 2023

Fixed bug - Passing options to UseStaticFiles breaks Blazor Server

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  1. Mapped endpoint to serve blazor.server.js from manifest resources.
  2. Deleted no longer used ConfigureStaticFilesOptions

Tested manually by running sample BlazorServerApp.

Fixes #19578

@surayya-MS surayya-MS requested a review from a team as a code owner January 5, 2023 14:38
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jan 5, 2023
app.UseStaticFiles(options);

var blazorEndpoint = endpoints.Map("/_framework/blazor.server.js", app.Build())
.WithDisplayName("Blazor static files");
Copy link
Member

Choose a reason for hiding this comment

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

This approach looks fairly plausible but would love to get @javiercn's take since he has focused a lot on how the server-side endpoints are set up and how static files are served.

Copy link
Member

Choose a reason for hiding this comment

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

This will work, the only "slight" concern that I have is that we loose the ability to server blazor.server.js independently of where we are serving the other endpoints, but I do not see a big drawback to it.

Essentially if you want to map blazor to a subdir, you need to call app.MapPath("subdir", sub => { sub.UseRouting(); sub.UseStaticFiles(); sub.UseEndpoints(e => e.MapComponentHub()); }, set the base path on the document accordingly.

There might have been other ways to make it work that might not work now, but I do not think they are critical.

@surayya-MS surayya-MS enabled auto-merge (squash) January 6, 2023 15:51
var blazorEndpoint = endpoints.Map("/_framework/blazor.server.js", app.Build())
.WithDisplayName("Blazor static files");

blazorEndpoint.Add((builder) => ((RouteEndpointBuilder)builder).Order = -1);
Copy link
Member

Choose a reason for hiding this comment

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

The -1 here makes it so that this endpoint has priority over other endpoints independent of their specificity, so this means that a controller or a file are not going to match over this endpoint.

With that said, I think we should be a bit more conservative here as people might also register their own routes with a custom order, which might cause those routes to match over this one. For that reason, we should do something similar but opposite to what we do in MapFallbackToPage and give it the lowest order possible (int.MinValue) so that we can guarantee under most circumstances, no other route will match over this one.

Copy link
Member

Choose a reason for hiding this comment

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

The only time this might cause issues is if someone customizes routing extensively, which I am not too worried about.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

I think the change looks great.

We have plenty of time to get feedback on this if we missed anything.

@surayya-MS surayya-MS merged commit de9b88a into dotnet:main Jan 9, 2023
@ghost ghost added this to the 8.0-preview1 milestone Jan 9, 2023
{
var options = new StaticFileOptions
{
FileProvider = new ManifestEmbeddedFileProvider(typeof(ComponentEndpointRouteBuilderExtensions).Assembly)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the removal of OnPrepareResponse = CacheHeaderSettings.SetCacheHeaders intentional?

Copy link
Member

Choose a reason for hiding this comment

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

Nope.

That's a good catch.

@surayya-MS can you take a look?

Copy link
Member Author

Choose a reason for hiding this comment

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

@campersau, thanks for noticing.

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
Development

Successfully merging this pull request may close these issues.

Passing options to UseStaticFiles breaks Blazor Server's static file service
4 participants