-
Notifications
You must be signed in to change notification settings - Fork 25.1k
Static files overhaul #35967
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
base: main
Are you sure you want to change the base?
Static files overhaul #35967
Conversation
This reverts commit cecbd3b.
Co-authored-by: Daniel Roth <[email protected]>
9ee972a
to
a2b8c81
Compare
}); | ||
``` | ||
|
||
When developing a server-side Blazor app and testing locally, see <xref:blazor/fundamentals/static-files#static-files-in-non-development-environments>. |
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 content in the Blazor static files article doesn't really seem to be Blazor specific. Does it really add anything that isn't already stated here? The static web assets support in ASP.NET Core is intended to be a generic feature that's not specific to Blazor, so I think most of that content should just be covered here.
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.
There were behavioral deltas when the article was set up. As a general principle, only deltas (or Blazor-focused scenarios) were covered by the Blazor-specific articles. I'll double-check and get back to you, but it might be Tuesday, given that I'm trying to get the Environments article work wrapped up before the holiday weekend.
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.
IDK why; but IWebHostBuilder.UseStaticWebAssets
may have never been covered in the main doc set's Static Files article. Instead (apparently), it was only covered in the Razor Pages Class Libraries article and left to a cross-link from the Static Files article to cover it ...
I'll fix this by moving the guidance on it from the Blazor article to the main doc set article. UPDATE: Done! 👍 What's in the RP article can stay, but I'll take a look at it and make sure that it's composed well. UPDATE: Yes. It seems OK as a passing point there.
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.
[EDIT by guardrex to number the points for responses]
@guardrex overflowed that static-files.md file and would like to give you small feedback to consider maybe:
-
if I read correctly, you do advise now for the core minimal api guide to use
MapStaticAssets()
andUseStaticFiles()
and to use theWebRootFileProvider
. what Information I do miss in there, can we call them multiple times? like if an extensions does call.UseStaticFiles()
and uses that provider and so on, which we might not see in our code since it might come by a nuget, should we expect problems? -
reffering to ln 94ff you advise us to call
UseHttpsRedirection()
which I did read before in some MVP posts, but do we need or should we also call Hsts depending on the Development Environment ? just wanting to point out, that if you advise the one of them and assuming from what I readed, that they are important, maybe the code Sample should just give a full block like also what we are required to call before the.UseStaticFiles()
and which we should keep in mind to put afterwards. for the httpsRedirection I readed that is e.g. important for when we use something like logging. I would miss a full ordered list somewhere in general also to such important list 👍 -
Line 189 ff: I did set about in a project that was targeting net9.0 and
.UseStaticFiles()
from suchwwwroot/Index.html
and did set without thinking about it could cause issues like you tell there to/
and instantly my asp net core wasm host was not longer building! this was not Razor in remark to your Warning there. the WebRootPath was also not set if you ask](https://github.com/DevTKSS/DevTKSS.MyManufacturerERP/blob/21bc8e2b3ed451bc90c6f5d8981faeddee7005a1/src/DevTKSS.MyManufacturerERP.Server/Program.cs#L147-L150)
created an issue on my repro
as soon as I removed that path, it was able to find these files again. but what I want to tell is, maybe there should be some remark about what would happen (maybe its an expected issue?) if we define such endpoint on the/
path? would be helpfull for non professinals -
line 227ff: this reads like a repeating and since your intro was about the assets usage to me this following section is absolutly confusing when I could not use the StaticFiles in .net9 or later for example.
e.g. lets think the reader wants to do a wasm app like me, but is not in deep in this topic, but the wasm app is not Blazor and does not call AddRazorPages. the Framework files are imported by exactly that.UseStaticFiles()
and I do not have a call to the assets. That documentation is now leaving me idea-less because, up from line 235 you seem to tell that the assets do not allow https headers, only static files, but the description from ln36 ff refers to Http Headers as feature + more of course. just as an example. -
ln 411-416
you introduced us in the first section that we need to call theMapStaticAssets()
before itsUseStaticFiles()
but now you switching it in the code sample??? and same here, I would miss any Dev environment condition for the redirection. -
line 723:
and suddenly its again possible to have DirectoryBrowsing withMapStaticAssets()
? thats not matching...
hoping this helps you improve it a bit 🚀 but for the rest, thanks! nice to see there is some action 👍
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.
-
You'll have to cross-link "for the core minimal api guide" because I don't know what that refers to. I'm not aware that a lib would set up Static File Middleware. I don't think that's a supported scenario, so you'd have to explain what you mean by that.
-
HSTS guidance is here. Because it's off-topic here, I don't think this article needs to comment on it. I'd prefer to allow the startup article show it first and then the middleware doc get into middleware ordering details. Technical review should turn up any problems here with regard to middleware/endpoint conventions ordering.
-
I defer to technical analysis by the product unit on it, and I'll add whatever coverage they suggest. However, they might end up asking you to open a separate issue on it.
-
Yes, you're probably correct because the opening paragraph doesn't say the important thing that the second paragraph states, namely that Static File Middleware can't do all of the wonderful things that Map Static Assets endpoint conventions can do.
I think I should work on that a bit more here now, so I'll make a few changes there for the next round of review.UPDATE: I reworked that section on the middleware (that only appears for the >=9.0 version of the article). -
I'm not sure about the last bit of your remark, but on the first part: Yes! That's what the product unit's guidance is for this particular approach for a large collection of assets. Javier will generally review that, but he also can comment on the second part of your remark to see if anything should be changed.
-
I haven't personally tested that scenario. However, I don't see any problem with a dedicated, non-
wwwroot
folder of assets for directory browsing working side-by-side with Map Static Asset endpoint conventions. I defer to Javier on technical review to say if there would be any problem with it working.
|
||
:::moniker range=">= aspnetcore-6.0" | ||
|
||
*The guidance in this section applies to Razor Pages and MVC apps. For guidance that applies to Blazor Web Apps, see <xref:blazor/fundamentals/static-files#serve-files-from-multiple-locations>.* |
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.
Isn't the guidance the same? Can we remove this duplication?
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.
That's what I meant about deltas. It wasn't the same IIRC, but I'll have to look. I'll check and get back to you ... probably Tuesday morning.
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.
Removed my last response. I was incorrect about what happened.
A dev had a problem getting our main doc set guidance with the composite file provider to work in a BWA with two physical file providers (which BTW works for Blazor Server, just not BWAs) ...
I asked the dev to open a PU issue to see what we needed to do for BWAs ...
However, the issue was transferred to the docs repo without comment. I closed the transferred issue to work from the original docs issue (32891).
I presumed that what the dev did to solve the problem was a valid approach, so that's what's in the section. Instead of using two physical file providers in a composite file provider, the Blazor content pitches passing app.Environment.WebRootFileProvider
as the primary provider ...
var secondaryProvider = new PhysicalFileProvider(
Path.Combine(builder.Environment.ContentRootPath, "AdditionalStaticAssets"));
app.Environment.WebRootFileProvider = new CompositeFileProvider(
app.Environment.WebRootFileProvider, secondaryProvider);
So, we still don't have an "official" Javier ruling 👨⚖️😄 on this delta (two physical providers in a composite doesn't work in a BWA). I'll leave the current coverage as it is for now, and then we can see on technical review what to do about BWA coverage for serving files from multiple locations.
[Note in passing that what I said in my stricken comment about the main doc set's section partially focusing the Image Tag Helper in this scenario is true. When I added the Blazor article coverage, I stripped that language out, so the Blazor article section may or may not be the right pitch on how to address the multiple locations scenario for BWAs, but it is better tailored to BWAs without the Tag Helper remarks. That's something that I'd try to address in the main doc set if we shed the Blazor article section after hearing back from Javier.]
Here are the two live sections for further analysis on their deltas to see if we should try combining them in the main doc set article ...
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.
Great start! I'd like to simplify the beginning of the static files doc as much as possible and gradually build up to more complicated and legacy scenarios. Dealing with static files is pretty common, so the basics need to come across as simple and straightforward. I also want to consolidate more of the general purpose static web asset content in the top-level static files doc instead of having a bunch of it duplicated in the Blazor. The Blazor static files doc really should only cover Blazor specific stuff, like the framework specific static web assets that it adds, the helpers it provides for referencing static web assets (Assets[]
), etc.
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Daniel Roth <[email protected]>
@danroth27 ... Ready for another look 👀. Notes on the changes ... Some discussion aboveThere are two points of discussion above ☝️. Focusing on Map Static Assets
For .NET 9 or later, Static Files Middleware is pushed down into the article immediately before where it's first needed. I also added a < .NET 9 bit for introductory content on Static File Middleware, which I should've placed earlier. Duplicate content
"A bunch"?? 😖😆 ... That was never the intention for Blazor's fundamentals coverage. Only in limited cases is there subject duplication. In ancient Blazor times 👴, I checked scenarios from the main doc set's articles to see where deltas required special coverage. That's what ended up in the Blazor Fundamentals node articles. Currently, I'm wondering why any fundamental coverage (or host/deploy ... or security) is covered in the Blazor node, given Blazor's preeminent position. MVC/RP stack nodes never carried such content the way that we're carrying Blazor content. Except for whatever we do with the Serve static files from multiple locations section (probably needs Javier's input, links are above ☝️), there's only one other section to address ... File mappings and static file options section. There was a major delta in versions prior to .NET 8. I'm updating the coverage now to drop the content in the Blazor article from .NET 8 forward. We're keeping the Blazor coverage for < .NET 8 due to the change in the way the Blazor script is served. Name of the middlewareBTW ... I think it should be "Static File Middleware," not "Files" (plural). The API isn't plural. I'm adjusting a handful of locations for this, including making sure that (by convention) that it's a proper noun. |
Fixes #35810
Fixes #35833
Dan ... I'll ping Javier for a technical review after I get your feedback. After that, I'll ping Wade and Tom.
Notes
dotnet/AspNetCore.Docs.Samples
repo. The sample apps were placed in that repo on Add static files sample apps AspNetCore.Docs.Samples#282. I'll delete them from this repo on this PR after reviews to avoid cluttering the diff.Internal previews