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

Conventional middleware class requirements. #12895

Merged
merged 3 commits into from
Jun 17, 2019
Merged

Conventional middleware class requirements. #12895

merged 3 commits into from
Jun 17, 2019

Conversation

serpent5
Copy link
Contributor

Fixes #11443.

I made a few changes here:

  • Changed the delegate example to use async/await to match the approach used in the middleware class.
  • Switched the example URL to use https and the template-generated default 5001 port for that.
  • Added an explanation of the requirements for a middleware class. All of these requirements are enforced by the hosting startup process, where exceptions are thrown if the requirements aren't met.
  • Moved the dependencies section up to below the requirements explanation I added. I think this follows on naturally.
  • Made the per-request dependencies section a child of the higher-level dependencies section.

TODO:

I don't know how to test the moniker ranges locally, so I couldn't update the note about InvokeAsync being available in ASP.NET Core 2.0+. Ideally, I'd like to know how to test this locally but alternatively, please make the relevant change. I imagine this being a different version of the relevant bullet for < 2.0, e.g.:

A public method named Invoke. In ASP.NET Core 2.0 or later, the name can also be InvokeAsync. This method must:

Copy link
Collaborator

@guardrex guardrex left a comment

Choose a reason for hiding this comment

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

Nice work ...

Update the ms.date on Line 7.

Copy link
Collaborator

@guardrex guardrex left a comment

Choose a reason for hiding this comment

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

One more nit.

@guardrex
Copy link
Collaborator

guardrex commented Jun 16, 2019

I don't know how to test the moniker ranges locally

Version the whole topic for 2.1+ (1.1 is about to go away, and 2.0 has already left the scene).

In the metadata between description and ms.author, place this line ...

monikerRange: '>= aspnetcore-2.1'

@serpent5
Copy link
Contributor Author

@guardrex Thanks for such prompt feedback. 🚀

I've (hopefully) made the relevant changes, as requested.

Copy link
Collaborator

@guardrex guardrex left a comment

Choose a reason for hiding this comment

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

❤️ I'm lov'in IT!™️

"I'm lov'in IT!" is a registered trademark of the McDonald's Corporation. 😄

@guardrex guardrex requested a review from Rick-Anderson June 17, 2019 18:55
@guardrex guardrex removed the request for review from Rick-Anderson June 17, 2019 22:14
@guardrex
Copy link
Collaborator

Let's go ahead and not disturb cats OOF. 🏖

@guardrex guardrex merged commit 035fdc8 into dotnet:master Jun 17, 2019
@serpent5 serpent5 deleted the serpent5-11443 branch June 17, 2019 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The requirements of the class to create a custom middleware are not well documented
2 participants