Move remaining feature interfaces into `Microsoft.AspNetCore.Http.Features` package and namespace #589

dougbu · 2016-03-17T23:47:47Z

related to ResponseCookies.Append() allocates lots, big lots #561
move required interfaces and classes down from Microsoft.AspNetCore.Http.Abstractions
move everything in the Microsoft.AspNetCore.Http.Features into their own directory

nit: remove transient dependencies listed in Microsoft.AspNetCore.Http.Abstractions's project.json

dougbu · 2016-03-17T23:48:37Z

/cc @lodejard @davidfowl
Will get reaction PRs (3 or 4) out later this evening.

davidfowl · 2016-03-18T04:31:46Z

src/Microsoft.AspNetCore.Http.Features/project.json

@@ -21,6 +21,7 @@
    "netstandard1.3": {
      "dependencies": {
        "System.Collections": "4.0.11-*",
+        "System.ComponentModel": "4.0.1-*",


Which type requires this?

IServiceProvidersFeature

FYI I moved this from src/Microsoft.AspNetCore.Http.Abstractions/project.json, same package that interface came from to this one.

davidfowl · 2016-03-18T04:32:29Z

We should look at renaming some of these interfaces on the second pass.

/cc @Tratcher

dougbu · 2016-03-18T04:46:35Z

We should look at renaming some of these interfaces on the second pass.

Just curious: Would the renaming be to add Http, to remove that, or something else?

In any case, I suggest filing a separate issue b/c such a renaming is unrelated to #561. That issue is specific to IResponseCookiesFeature.

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

davidfowl · 2016-03-18T12:34:29Z

Just curious: Would the renaming be to add Http, to remove that, or something else?

Probably adding http to some of them (the ones that relate to http things like form)

Tratcher · 2016-03-18T15:24:40Z

Having a /Features directory inside the Features package is redundant.

dougbu · 2016-03-18T15:30:03Z

Having a /Features directory inside the Features package is redundant.

We're supposed to have a directory per namespace and @Eilon asked me to clean up this particular mix.

davidfowl · 2016-03-18T15:56:12Z

I have to agree with @Tratcher, why is it in a features folder?

dougbu · 2016-03-18T15:58:31Z

why is it in a features folder?

Huh? Are you suggesting creating a .Root folder for everything that's in the Microsoft.AspNetCore.Http namespace and putting that below the .Features files? Or are you suggesting we ignore @Eilon's guidance?

Tratcher · 2016-03-18T16:31:10Z

The guideline is to use folders for sub-namespaces. There's no guideline for components in parent namespaces. I think it's fine to put everything in the base directory in this case.

dougbu · 2016-03-18T16:38:53Z

it's fine to put everything in the base directory in this case

That means mixing Microsoft.AspNetCore.Http and Microsoft.AspNetCore.Http.Features in the root directory. Put another way, Microsoft.AspNetCore.Http.Features is a sub-namespace relative to the files in the root directory.

Note moving, say, IFormCollection into Microsoft.AspNetCore.Http.Features is a non-starter. It's more general than what we have in the sub-namespace. (Actually, should probably move ISession and perhaps WebSocketAcceptContext up into the root namespace and directory.)

@Eilon ?

Eilon · 2016-03-18T17:16:23Z

Oh I didn't realize the files would effectively be in a ...Features.Features folder. That's worse.

Just leave it top-level then.

dougbu · 2016-03-18T17:17:18Z

Just leave it top-level then.

🆗

…tures` package and namespace - related to #561 - move required interfaces and classes down from `Microsoft.AspNetCore.Http.Abstractions` - move everything in the `Microsoft.AspNetCore.Http.Features` into their own directory nit: remove transient dependencies listed in `Microsoft.AspNetCore.Http.Abstractions`'s `project.json`

…o root directory

dougbu · 2016-03-18T17:26:04Z

🆙📅

Tratcher · 2016-03-18T17:57:42Z

Why not include #590?

dougbu · 2016-03-18T18:00:26Z

Why not include #590?

Because that class and interface were already in the Microsoft.AspNetCore.Http.Features package and the change suggested in #590 was unrelated. Plus I didn't know if #590 was controversial for some reason.

Tratcher · 2016-03-18T18:07:06Z

Meh, do it so you only have to make one pass of updates.

dougbu · 2016-03-18T18:08:56Z

do it

🆗

….Http` namespace - #590

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

…espace - see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu · 2016-03-19T05:46:20Z

🆙📅

davidfowl · 2016-03-19T15:21:10Z

src/Microsoft.AspNetCore.Owin/OwinEnvironment.cs

 using Microsoft.AspNetCore.Http.Features.Authentication;
 using Microsoft.AspNetCore.Http.Features.Authentication.Internal;
+using Microsoft.AspNetCore.Http.Features.Internal;


We still have internal features?

Implementations

Those should go into an internal folder...

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http/Features/HttpConnectionFeature.cs

Tratcher · 2016-03-19T16:08:49Z

davidfowl · 2016-03-19T16:46:25Z

I don't think these should be considered internal

dougbu · 2016-03-19T20:18:49Z

I don't think these should be considered internal

@davidfowl do you mean I should move all of the implementations into the Microsoft.AspNetCore.Http.Features namespace? And, unlike the interfaces, no files will be moved in the process? (From what I've seen, the implementations are referenced much less than the interfaces. But they're definitely used, most obviously in KestrelHttpServer.)

- see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

davidfowl · 2016-03-21T04:02:09Z

@davidfowl do you mean I should move all of the implementations into the Microsoft.AspNetCore.Http.Features namespace? And, unlike the interfaces, no files will be moved in the process? (From what I've seen, the implementations are referenced much less than the interfaces. But they're definitely used, most obviously in KestrelHttpServer.)

We can file a bug for this.

dougbu · 2016-03-21T04:46:39Z

We can file a bug for this.

#592

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

- see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

…espace - see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu · 2016-03-21T16:13:37Z

6f24508

dnfclas added the cla-already-signed label Mar 17, 2016

davidfowl reviewed Mar 18, 2016
View reviewed changes

dougbu added a commit to aspnet/Hosting that referenced this pull request Mar 18, 2016

React to HttpAbstractions change: No features in .Internal namespace

af49efe

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/KestrelHttpServer that referenced this pull request Mar 18, 2016

React to HttpAbstractions change: No features in .Internal namespace

bf4c1fc

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/Security that referenced this pull request Mar 18, 2016

React to HttpAbstractions change: No features in .Internal namespace

91c4d47

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added 2 commits March 18, 2016 10:24

PR comments: Move Microsoft.AspNetCore.Http.Features files back int…

10aa15c

…o root directory

dougbu force-pushed the dougbu/feature.interfaces.561 branch from b026be4 to 10aa15c Compare March 18, 2016 17:25

Move ISession and WebSocketAcceptContext to `Microsoft.AspNetCore…

ad9bcb0

….Http` namespace - #590

dougbu added a commit to aspnet/KestrelHttpServer that referenced this pull request Mar 19, 2016

React to HttpAbstractions change: No features in .Internal namespace

f13c418

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/Security that referenced this pull request Mar 19, 2016

React to HttpAbstractions change: No features in .Internal namespace

9361960

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/Session that referenced this pull request Mar 19, 2016

React to HttpAbstractions change: WebSocketAcceptContext in new nam…

dee2914

…espace - see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/MusicStore that referenced this pull request Mar 19, 2016

React to HttpAbstractions change: WebSocketAcceptContext in new nam…

0cdc7f7

…espace - see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

This was referenced Mar 19, 2016

React to HttpAbstractions change: ISession in new namespace aspnet/Session#101

Closed

React to HttpAbstractions change: WebSocketAcceptContext in new namespace aspnet/MusicStore#614

Merged

davidfowl reviewed Mar 19, 2016
View reviewed changes

dougbu added a commit to aspnet/Session that referenced this pull request Mar 19, 2016

React to HttpAbstractions change: ISession in new namespace

7e32cf4

- see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu mentioned this pull request Mar 20, 2016

Use pooled StringBuilder to reduce allocations when adding response cookies #591

Closed

dougbu mentioned this pull request Mar 21, 2016

Move feature implementations into Microsoft.AspNetCore.Http.Features namespace #592

Closed

dougbu added a commit to aspnet/Hosting that referenced this pull request Mar 21, 2016

React to HttpAbstractions change: No features in .Internal namespace

bb3809d

- see issue aspnet/HttpAbstractions#561 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/Session that referenced this pull request Mar 21, 2016

React to HttpAbstractions change: ISession in new namespace

5371e13

- see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu added a commit to aspnet/WebSockets that referenced this pull request Mar 21, 2016

React to HttpAbstractions change: WebSocketAcceptContext in new nam…

0f9e4eb

…espace - see issue aspnet/HttpAbstractions#590 and pull aspnet/HttpAbstractions#589

dougbu closed this Mar 21, 2016

Move remaining feature interfaces into Microsoft.AspNetCore.Http.Features package and namespace #589

Move remaining feature interfaces into Microsoft.AspNetCore.Http.Features package and namespace #589

Uh oh!

Conversation

dougbu commented Mar 17, 2016

Uh oh!

dougbu commented Mar 17, 2016

Uh oh!

davidfowl Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

dougbu Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

dougbu Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

davidfowl commented Mar 18, 2016

Uh oh!

Tratcher commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

davidfowl commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

Tratcher commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

Eilon commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

Tratcher commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

Tratcher commented Mar 18, 2016

Uh oh!

dougbu commented Mar 18, 2016

Uh oh!

dougbu commented Mar 19, 2016

Uh oh!

davidfowl Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

Tratcher Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

davidfowl Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

Tratcher Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Mar 19, 2016

Uh oh!

davidfowl commented Mar 19, 2016

Uh oh!

dougbu commented Mar 19, 2016

Uh oh!

davidfowl commented Mar 21, 2016

Uh oh!

dougbu commented Mar 21, 2016

Uh oh!

dougbu commented Mar 21, 2016

Uh oh!

Uh oh!

Move remaining feature interfaces into `Microsoft.AspNetCore.Http.Features` package and namespace #589

Move remaining feature interfaces into `Microsoft.AspNetCore.Http.Features` package and namespace #589