Conversation
There was a problem hiding this comment.
Pull request overview
This PR performs a comprehensive overhaul of the middleware article and related documentation to make it more Blazor-friendly. The changes include removing obsolete include files, updating cross-references, deleting old snapshot code files, adding a new SVG diagram, and inlining code samples that were previously in separate files.
Changes:
- Removes
code-comments-loc.mdandForwardedHeaders.mdinclude files and replaces their usage with inline content - Deletes multiple snapshot code files and mermaid diagram files that are no longer needed
- Adds new
filter-pipeline-3.svgdiagram for the filters article - Updates cross-reference links to use consistent fragment identifiers (e.g.,
#middleware-orderinstead of#order) - Inlines code samples throughout the middleware article for better maintainability
Reviewed changes
Copilot reviewed 53 out of 55 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| aspnetcore/includes/code-comments-loc.md | Deleted include file for code comments localization notice |
| aspnetcore/includes/ForwardedHeaders.md | Deleted include file for Forwarded Headers Middleware ordering |
| aspnetcore/fundamentals/middleware/index/includes/*.md | Deleted multiple versioned middleware article includes |
| aspnetcore/fundamentals/middleware/index/snapshot/*.cs | Deleted snapshot code files |
| aspnetcore/mvc/controllers/filters/_static/filter-pipeline-3.svg | Added new SVG diagram showing filter pipeline |
| aspnetcore/fundamentals/middleware/index/includes/index3-7.md | Inlined code samples and removed include references |
| Multiple .md files | Updated xref links and removed include file references |
|
@BrennanConroy ... Hi and Happy New Year! 🥳 ... Could you answer a question for me about what Rick added to the Middleware article at ... Prefer ... relative to the discussion on ... The current article section is guiding devs to the overload almost as an afterthought, but it looks like you intend to recommend it over the earlier non- If that's correct, I'll make the updates on this PR, which is an overhaul of the whole article. This has come up here because we're in the process of Blazorfying™ 😄 the main doc set's Fundamentals articles, and I noticed this potential problem in passing. If I'm headed in the right direction, is it best to keep both overloads, describing and recommending the new overload first (are there gotchas 😈 where the prior overload is best)? Alternatively, would it be better to not describe the earlier (non- cc: @danroth27 |
|
We should probably promote it as the only |
|
Thanks, @BrennanConroy ... Will do. 👍 |
|
@tdykstra ... It's ready for review now. 🎉 ... AND ....Let me know if you want me to get rid of the .NET 3 to 7 INCLUDES file and roll the content into each section with versioning in one markdown file for no broken bookmarks in prior release content and a lot less duplicate content. I can work it out if you want, and we can pick back up on Monday with the review. |
We adopted the include files for different versions because when everything was in one file people tend to overlook the prior versions when editing an article. Maybe a warning in a comment near the top of the file would address that. Thoughts? |
|
I understand what you mean about readers not scrolling to the bottom to edit other entire-article versioned markdown; but to avoid the broken bookmark problem and not have duplicate headings, I set up versioned content entirely within sections. There are no duplicate sections (heading The downside is version-tag flipping a bit here and there, which we've seen become very painful at times (e.g., the Kestrel article several years ago prior to adopting the INCLUDES file approach there). Fortunately, it's only been a minor problem in a few Blazor articles ... it's been manageable thus far. If you want to keep the current setup, the PR is ready for final review. I would just need to add the commented remark at the top of the file. However, that sounds like something that the team might do separately and uniformly. This is but one of several dozen articles that would receive such remarks. |
|
@tdykstra ... Did you decide what you want to do with the INCLUDES file on this one? I'm working on the Options article overhaul, and I discovered that it will be fairly easy on that one to drop the INCLUDES file, so I'm doing that and will have that PR up perhaps on Monday. Anyway ... do you know if you want to keep it for the Middleware article? I'm not sure if it would be easy or hard to do, but I can try 😄. Also, do you want to have a separate issue and PR to apply a global remark to the articles that will continue to use INCLUDES files? It seems like such a remark wouldn't be added to just one article like this in an ad hoc fashion (if this one ends up keeping its INCLUDES file). |
Go ahead and try
Sounds good. |
|
@tdykstra ... Lick'd IT! 🐮👅 ...... I was able to drop the INCLUDES file ...
This is ready for review. |
3 participants
Fixes #35805
I performed a general pass as I seek to make it Blazor friendly.
There's one question that I have in here of a technical nature: For
Usedelegate overload that passes theHttpContexttonextfor performance, why aren't we pitching that overload in all general use cases if it provides a perf improvement? Looking at it another way, what are the caveats/gotchas 😈 that we can state in the section for when it shouldn't be called?Do you want to continue with the INCLUDES file versioning approach? 👂 If not, I'll roll the 3-7 file in here by section with versioning to get it down to one file (and no broken links in prior release content).
I'll remove the inline reviewer comments after we're done with reviews before merging.
Internal previews
Toggle expand/collapse