Target netstandard2.1 for SignalR client #10540

BrennanConroy · 2019-05-25T19:48:44Z

BrennanConroy

I didn't change a couple libraries like Http.Connections.Common because the reason we cross-targeted that one was to not pull in the System.Text.Json package on netcoreapp3.0

BrennanConroy · 2019-05-25T19:51:29Z

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

-#else
-            var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
-            if (!isWindows)
+            try


I don't think we can do a compile time check here, for example, someone running on netcoreapp3.0 but targeting the netstandard2.0 library on windows 7 will have access to the managed websockets impl, but if we just hardcoded netstandard2.0 as not having websockets then a scenario that should work will not work.

This kinda sucks....

BrennanConroy · 2019-05-25T19:52:30Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

@@ -49,7 +49,7 @@ public partial class HubConnection
        };

        private static readonly MethodInfo _sendStreamItemsMethod = typeof(HubConnection).GetMethods(BindingFlags.NonPublic | BindingFlags.Instance).Single(m => m.Name.Equals(nameof(SendStreamItems)));
-#if NETCOREAPP3_0
+#if NETSTANDARD2_1


Maybe all the #if NETSTANDARD2_1 checks should be #if !NETSTANDARD2_0, that way we might never have to update them in the future.

Doubt it matters. We can't be sure at this point when we'll drop .NET Standard 2.0 support, update 2.1 to something newer, add a new TFM, …

What you can do is have ifdefs based on the available feature.

For example, this test could be #if HAVE_ASYNCENUMERABLE. Then in the csproj against NETSTANDARD2_1 you would define HAVE_ASYNCENUMERABLE.

Now when a new target is added in the future, e.g. netcoreapp50, you just add it to the csproj and set the required flags. No need to update individual source code files.

Example: https://github.com/JamesNK/Newtonsoft.Json.Bson/blob/a1db92678e7e72eb733f37079f3a93bfb6215338/Src/Newtonsoft.Json.Bson/Newtonsoft.Json.Bson.csproj#L36-L47

We know that all targets after netstandard2.0 will have these new APIs so I don't see why we'd add extra upkeep when a simple !NETSTANDARD2_0 check will do. And in the future if we remove netstandard2.0 compatibility then we just remove the #if entirely.

If for some crazy reason we do have future targets that remove features like IAsyncEnumerable then we can revisit and add feature flags.

Future targets will never remove IAsyncEnumerable but future targets will add new features that you will likely want to use. With multiple targets things start getting complicated quickly...

A new feature in 3.0: #if !(NETSTANDARD2_0 || NETSTANDARD2_1)
A new feature in 4.0: #if !(NETSTANDARD2_0 || NETSTANDARD2_1 || NETSTANDARD3_0)
etc

Meanwhile using feature flags are a single test that and easy to read and all visible in the csproj to see what each target supports.

BrennanConroy · 2019-05-25T19:53:27Z

src/SignalR/common/Shared/AsyncEnumerableAdapters.cs

@@ -24,13 +24,26 @@ public static IAsyncEnumerable<T> MakeCancelableTypedAsyncEnumerable<T>(IAsyncEn
            return new CancelableTypedAsyncEnumerable<T>(asyncEnumerable, cts);
        }

+#if NETCOREAPP3_0
        public static async IAsyncEnumerable<object> MakeAsyncEnumerableFromChannel<T>(ChannelReader<T> channel, [EnumeratorCancellation] CancellationToken cancellationToken = default)
        {
            await foreach (var item in channel.ReadAllAsync(cancellationToken))


channel.ReadAllAsync isn't available on netstandard2.1, is this something we should ask CoreFX to provide or...?

We might get this for free with https://github.com/dotnet/corefx/issues/34963

dougbu · 2019-05-27T19:49:03Z

Directory.Build.targets

+      <!-- Only update the default runtime version for preview builds. -->
+      <DefaultRuntimeFrameworkVersion Condition="'%(TargetFramework)' == '$(DefaultNetCoreTargetFramework)' and '$(IsServicingBuild)' != 'true'">$(MicrosoftNETCoreAppPackageVersion)</DefaultRuntimeFrameworkVersion>
+      <!-- Only update the targeting pack version for preview builds. -->
+      <TargetingPackVersion Condition="'%(TargetFramework)' == '$(DefaultNetCoreTargetFramework)' and '$(IsServicingBuild)' != 'true'">$(MicrosoftNETCoreAppPackageVersion)</TargetingPackVersion>


Suggest moving the consistent Condition="'%(TargetFramework)' == '$(DefaultNetCoreTargetFramework)'" to the <KnownFrameworkReference> element as is done below.

dougbu · 2019-05-27T19:57:34Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

@@ -49,7 +49,7 @@ public partial class HubConnection
        };

        private static readonly MethodInfo _sendStreamItemsMethod = typeof(HubConnection).GetMethods(BindingFlags.NonPublic | BindingFlags.Instance).Single(m => m.Name.Equals(nameof(SendStreamItems)));
-#if NETCOREAPP3_0
+#if NETSTANDARD2_1


Doubt it matters. We can't be sure at this point when we'll drop .NET Standard 2.0 support, update 2.1 to something newer, add a new TFM, …

src/SignalR/clients/csharp/Client.Core/ref/Microsoft.AspNetCore.SignalR.Client.Core.csproj

BrennanConroy · 2019-05-29T15:49:10Z

We might be delaying this until preview7.

I'll probably spin up a PR with just the infrastructure changes.

analogrelay · 2019-05-29T20:05:08Z

At this point unless we're super clear on what we're doing, deferring to preview 7 seems safest :).

BrennanConroy · 2019-06-06T17:32:14Z

🆙 📅 This should be good now

davidfowl · 2019-06-06T17:45:31Z

...p/Http.Connections.Client/ref/Microsoft.AspNetCore.Http.Connections.Client.netstandard2.1.cs

+        public string TransportType { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
+    }
+}
+namespace Microsoft.AspNetCore.Http.Connections.Client.Internal


We need to make these internal 😄

BrennanConroy · 2019-06-10T15:57:42Z

Reviewers?

My main concern with this is that we only test the netcoreapp3.0 versions of the binaries.

analogrelay

I only see one place where we are conditional on netcoreapp3.0 without also supporting netstandard2.1, what's the value of keeping that TFM in place? Is there actually different code we want to write?

analogrelay · 2019-06-10T17:42:33Z

src/SignalR/common/Shared/AsyncEnumerableAdapters.cs

@@ -24,13 +24,26 @@ public static IAsyncEnumerable<T> MakeCancelableTypedAsyncEnumerable<T>(IAsyncEn
            return new CancelableTypedAsyncEnumerable<T>(asyncEnumerable, cts);
        }

+#if NETCOREAPP3_0


Is this not possible in NETSTANDARD2_1?

channel.ReadAllAsync isn't in netstandard2.1 currently. And they seem to not be interested in moving corefx to netstandard2.1 currently. https://github.com/dotnet/corefx/issues/34963

If you want an API added to netstandard2.1 you should request it on dotnet/standard, e.g. dotnet/standard#1144

(as far as I know. Worst case they can only say no 😋)

Channels isn't in netstandard at all, it's a library that targets netstandard2.0 (and doesn't cross-target 2.1).

My understanding of that issue though is that corefx isn't going to bulk-update to target 2.1. I think it doesn't preclude targeting 2.1 in a particular assembly.

If we really think netstandard2.1 is worth it here then I guess this is why we need to triple-target. It just seems counter-productive to keep doing this.

This is what the world of developing libraries that run everywhere is like.

btw I still think you should use feature flags. If makes this stuff easier #10540 (comment)

#VoiceOfExperience

BrennanConroy · 2019-06-11T02:17:27Z

what's the value of keeping that TFM in place?

@davidfowl is wanting to put the client in the shared framework.

analogrelay · 2019-06-11T02:29:53Z

is wanting to put the client in the shared framework.

Does that require the netcoreapp3.0 TFM? I wouldn't think it should.

davidfowl · 2019-06-11T05:16:04Z

Does that require the netcoreapp3.0 TFM? I wouldn't think it should.

I want no unnecessary deps when running on netcoreapp3.0 as well.

analogrelay · 2019-06-11T05:30:17Z

I want no unnecessary deps when running on netcoreapp3.0 as well.

What unnecessary deps are there?

davidfowl · 2019-06-11T05:40:44Z

What unnecessary deps are there?

System.Threading.Channels is in the 3.0 shared framework.

I was also assuming we'd use the Microsoft.Bcl.AsyncInterfaces on ns2.0

analogrelay · 2019-06-11T15:26:19Z

Right but why would embedding this package in the shared framework be a problem if all it's deps are in the shared fx? I don't understand how a separate build actually solves what seems to be a deployment problem. When this is embedded in the shared framework, it's dependencies will already be there so it should only add this single assembly. Even if we use the netstandard2.1 version that would be true.

The comment above about ChannelReader.ReadAllAsync not being present in netstandard2.1 may be the real reason we should triple-target but I don't see how your concerns warrant triple-targeting...

analogrelay · 2019-06-12T20:47:00Z

src/SignalR/common/Shared/AsyncEnumerableAdapters.cs

@@ -32,9 +32,11 @@ await foreach (var item in channel.ReadAllAsync(cancellationToken))
            }
        }
 #else
+        // System.Threading.Channels.ReadAllAsync() is not available on netstandard2.0 and netstandard2.1
+        // But this is the exact same code that it uses


Was just going to ask for this after our convo 👍

BrennanConroy · 2019-06-13T00:14:53Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=222484

aspnet-hello · 2019-06-13T00:17:30Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2200

halter73 · 2019-06-13T03:04:48Z

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

                return true;
            }
-            else
+            catch


Do we know what get thrown in practice?

NotSupportedException

I figured. This is gross, but catching only the NotSupportedException is probably even worse.

Do we cache this now?

BrennanConroy · 2019-06-13T04:18:02Z

@aspnet/build Can someone merge please. Seems some kind of AzDo and GitHub integration is broken, everything is passing if you manually check.

BrennanConroy added the area-signalr Includes: SignalR clients and servers label May 25, 2019

BrennanConroy added this to the 3.0.0-preview6 milestone May 25, 2019

BrennanConroy commented May 25, 2019

View reviewed changes

BrennanConroy marked this pull request as ready for review May 27, 2019 19:18

BrennanConroy requested review from analogrelay, dougbu, halter73, mikaelm12 and a team as code owners May 27, 2019 19:18

dougbu reviewed May 27, 2019

View reviewed changes

davidfowl reviewed May 27, 2019

View reviewed changes

src/SignalR/clients/csharp/Client.Core/ref/Microsoft.AspNetCore.SignalR.Client.Core.csproj Show resolved Hide resolved

BrennanConroy modified the milestones: 3.0.0-preview6, 3.0.0-preview7 May 29, 2019

analogrelay mentioned this pull request Jun 4, 2019

Implement IAsyncDisposable in Connection.Abstractions #10607

Closed

2 tasks

BrennanConroy force-pushed the brecon/21 branch from 9fdaafc to e548f62 Compare June 5, 2019 03:53

davidfowl reviewed Jun 6, 2019

View reviewed changes

analogrelay reviewed Jun 10, 2019

View reviewed changes

BrennanConroy force-pushed the brecon/21 branch from de0ccfa to a567fee Compare June 11, 2019 19:00

BrennanConroy requested review from jkotalik and Tratcher as code owners June 11, 2019 22:03

BrennanConroy force-pushed the brecon/21 branch from aa37e50 to 449c049 Compare June 12, 2019 18:39

analogrelay reviewed Jun 12, 2019

View reviewed changes

davidfowl approved these changes Jun 13, 2019

View reviewed changes

BrennanConroy and others added 18 commits June 12, 2019 19:28

stash

7eacb9e

fin

dfeb95b

back to 3.0

611829c

runtime check

76b9a0f

version details

4c2cdc5

Ref assemblies

10c1df7

cleanup

fd54ac5

three-fm

2a07a90

fixup

befa189

update

a9213c3

fallback

6f4eca8

add bcl.asyncinterfaces

4983ad4

connection abstractions

2ac143a

fix ref?

f008dcd

packageversion suffix

c86b9a6

rm netcoreapp3.0

911df50

really....

07a3d46

remove more 3.0

c050fc7

BrennanConroy force-pushed the brecon/21 branch from 3a6a61e to c050fc7 Compare June 13, 2019 02:30

halter73 reviewed Jun 13, 2019

View reviewed changes

halter73 approved these changes Jun 13, 2019

View reviewed changes

BrennanConroy mentioned this pull request Jun 13, 2019

Convert SignalR EventSource usage to new pattern #11130

Merged

dougbu merged commit f989dac into master Jun 13, 2019

ghost deleted the brecon/21 branch June 13, 2019 05:00

Target netstandard2.1 for SignalR client #10540

Target netstandard2.1 for SignalR client #10540

Uh oh!

Conversation

BrennanConroy commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrennanConroy commented May 29, 2019

Uh oh!

analogrelay commented May 29, 2019

Uh oh!

BrennanConroy commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy commented Jun 10, 2019

Uh oh!

analogrelay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy commented Jun 11, 2019

Uh oh!

analogrelay commented Jun 11, 2019

Uh oh!

davidfowl commented Jun 11, 2019

Uh oh!

analogrelay commented Jun 11, 2019

Uh oh!

davidfowl commented Jun 11, 2019

Uh oh!

analogrelay commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

BrennanConroy commented May 25, 2019 •

edited

Loading

JamesNK May 27, 2019 •

edited

Loading

analogrelay left a comment •

edited

Loading

JamesNK Jun 11, 2019 •

edited

Loading