Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Add UseWhenExtensions and UseWhenExtensionsTests #663

Closed
wants to merge 1 commit into from

Conversation

tuespetre
Copy link
Contributor

Addresses #351

@dnfclas
Copy link

dnfclas commented Jun 30, 2016

Hi @tuespetre, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@kevinchalet
Copy link
Contributor

Nice, I had planned to add UseWhen to the aspnet-contrib packages, but if it's part of ASP.NET Core, that's cool too 😄

IMHO, using Predicate instead of a delegate accepting a HttpContext prevents tons of useful scenarios.

Here's my own version of UseWhen, reviewed by @Tratcher last year: https://gist.github.com/PinpointTownes/6d0492cd3d1b3a64a670

@Tratcher Tratcher self-assigned this Jun 30, 2016
@tuespetre
Copy link
Contributor Author

tuespetre commented Jun 30, 2016

Haha, reinvented wheels :)

The 'Predicate' is just an alias for Func<HttpContext,bool>. I just copied the MapWhenExtensions file and changed it (same for the tests.)

@kevinchalet
Copy link
Contributor

kevinchalet commented Jun 30, 2016

The 'Predicate' is just an alias for Func.

Sure, but this doesn't allow you to support path-specific middleware registrations.

E.g

// Create a new branch where the registered middleware will be executed only for non API calls.
app.UseWhen(context => !context.Request.Path.StartsWithSegments("/api"), branch => {
    branch.UseCookieAuthentication(new CookieAuthenticationOptions { ... });
});

@kevinchalet
Copy link
Contributor

Okay, my bad. I had not realized you had added an alias at the top of the file 😄

@tuespetre
Copy link
Contributor Author

Oops, GitHub took my brackets for HTML. I'm on my phone and don't seem to have the backtick key needed for an inline code span. That should have read 'Func(HttpContext,bool)'

@kevinchalet
Copy link
Contributor

Defining custom aliases with common names (like Predicate) is a nice trap 😄

On a related, an async overload might be nice too.

/// <param name="predicate">Invoked with the request environment to determine if the branch should be taken</param>
/// <param name="configuration">Configures a branch to take</param>
/// <returns></returns>
public static IApplicationBuilder UseWhen(this IApplicationBuilder app, Predicate predicate, Action<IApplicationBuilder> configuration)
Copy link
Member

Choose a reason for hiding this comment

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

This covers the if (condition) { } scenario, but is there also a need for if (condition) { } else { } or even if (condition) { } elseif (condition) { } elseif (condition) { } else { } ?

Copy link
Member

Choose a reason for hiding this comment

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

if (condition) { } else { } can be done as if (condition) { } if (!condition) { }, but it may be inefficient to evaluate the conditional twice.

if (condition) { } elseif (condition) else { } can be done as if (condition) { } else { if (condition) { } else { } }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tratcher as far as I see it this fills the need of the original issue, if the additional use case(s) ever become in demand they can be designed then...

@Tratcher
Copy link
Member

Tratcher commented Jul 5, 2016

Rebased and merged, thanks.

@Tratcher Tratcher closed this Jul 5, 2016
@Tratcher Tratcher added this to the 1.1.0 milestone Jul 5, 2016
@tuespetre
Copy link
Contributor Author

Thanks!

branchBuilder.Run(main);
var branch = branchBuilder.Build();

return async context =>
Copy link
Contributor

@kevinchalet kevinchalet Jul 5, 2016

Choose a reason for hiding this comment

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

nit: the async/await could have been removed here to save a state machine allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱 true, true

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants