aspnet / EventNotification

[Archived] Event notification system for broadcasting application state and configuration. Project moved to https://github.com/aspnet/Extensions
Apache License 2.0
45 stars 26 forks source link

Target .NET Standard 2.0 #70

Closed natemcmaster closed 7 years ago

natemcmaster commented 7 years ago

Part of https://github.com/aspnet/Home/issues/2045

Notable changes:

Background: System.Reflection.Emit was (perhaps mistakenly) part of .NET Standard 1.x, but was removed from 2.0. These APIs are only available when targeting .NET Core 2.0 and .NET Framework 4.6.1 directly.

natemcmaster commented 7 years ago

After further discussion and internal conversation, we decided not to target .NET Standard 2.0. Look for a followup that will change this package to target only .NET Core 2.0 and .NET Framework 4.6.1. The decision comes primarily from this package's heavy dependency on System.Reflection.Emit. Without it, there isn't much useful API. Therefore, we'll only target frameworks that support refemit.

terrajobst commented 7 years ago

The decision comes primarily from this package's heavy dependency on System.Reflection.Emit. Without it, there isn't much useful API. Therefore, we'll only target frameworks that support refemit.

That makes no sense to me. You can target .NET Standard and reference System.Reflection.Emit. It just means you only run on .NET implementations that support runtime code gen.

Yes, this means folks can install the package into UWP without a build error which will blow up at runtime. However:

  1. You can work this around with a custom .targets file that fails in UWP
  2. We should address this behavior in NuGet

Dropping .NET Standard support entirely makes this package unusable* for the vast majority of libraries.

*Yes, folks will be able to consume this from .NET Standard 2.0 thanks to the compat shim, but this package violates what we tell customers to do: if you want to share code between .NET Core and .NET Framework, you should target .NET Standard.

natemcmaster commented 7 years ago

You can target .NET Standard and reference System.Reflection.Emit.

@terrajobst at the time this was discussed, we were told mixing the 4.3.0 System.* packages with NETStandard.Library 2.0.0 was not okay. There is no System.Reflection.Emit package 4.4.0 and the APIs in this namespace are not in ns2.0.

See also this discussion: https://github.com/dotnet/corefx/issues/17746

terrajobst commented 7 years ago

Odd. I tried this other day it worked as expected. System.Reflection.Emit 4.3.0 targets .NET Standard 1.1, so you can consume it from .NET Standard 2.0 -- and that version is from November 2016, so older than the linked issue.

@weshaggard?

bording commented 7 years ago

@terrajobst After my conversation with you on twitter, we did end up moving to .NET Standard 2.0 + Reflection.Emit 4.30 packages, so if there's actually a problem with that combination, I think it needs to be announced very publicly.

terrajobst commented 7 years ago

In other words, "if it works, we keep quiet, otherwise I expect Immo to publicly apologize for his bad guidance". Seems fair 😄

bording commented 7 years ago

Well, didn't mean it quite like that of course 😄 but something needs to be done to actually block the combination if it's not meant to be used together.

Right now there is zero indication that it could even be a problem. The packages happily install and so far seem to be working correctly.

terrajobst commented 7 years ago

Synced with @weshaggard offline. The problem is that we can't build the System.Reflection.Emit reference assembly cleanly in CoreFX because it effectively needs to gain access to internal constructors in order to derive from TypeInfo. The current 4.3 version basically hacked it, but should work everywhere.

The real downer is the experience I mentioned earlier: no build errors. We need to fix that.