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

fix: use netstandard 2.0 compatible version of splat #1579

Merged
merged 28 commits into from Apr 30, 2018

Conversation

Projects
None yet
4 participants
@ghuntley
Copy link
Member

ghuntley commented Mar 12, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Fix

What is the current behavior? (You can also link to an open issue here)

Uses Splat v2.0 which isn't compatible with netstandard2.0

What is the new behavior (if this is a feature change)?

Uses Splat v3.0 which is compatible with netstandard2.0

What might this PR break?

Folks who are using pre-release builds will be automatically upgraded to splat3.0 when they upgrade their binary dependancy if they are using Visual Studio for Windows and packageref.

Other information:

@ghuntley ghuntley added the bug label Mar 12, 2018

@ghuntley ghuntley added this to the 8.0.0 milestone Mar 12, 2018

ghuntley added some commits Apr 24, 2018

@@ -57,7 +57,7 @@

<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">
<Compile Include="Platforms\netcoreapp1.0\**\*.cs" />
<PackageReference Include="System.Runtime.Serialization.Primitives" Version="4.1.1" />
<PackageReference Include="System.Runtime.Serialization.Primitives" Version="4.3.0" />

This comment has been minimized.

@onovotny

onovotny Apr 24, 2018

Member

This causes package downgrade warnings. Please don't change this for netcoreapp1.x.

This comment has been minimized.

@olevett

This comment has been minimized.

@onovotny

onovotny Apr 24, 2018

Member

That's because splat is broken:

https://github.com/reactiveui/splat/blob/3.0.0/src/Splat/Splat.csproj#L19

Should not be referencing 4.3 there either.

Best solution overall is to remove netcoreapp1.0 and netstandard1.x. min version should be net461 and netstandard2.0.

@onovotny
Copy link
Member

onovotny left a comment

Please fix the primitives version

olevett added some commits Apr 24, 2018

ghuntley added some commits Apr 25, 2018

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 25, 2018

The ReactiveUI targets need to also be bumped to netstandard20.

C:\projects\reactiveui\src\ReactiveUI.Testing\ReactiveUI.Testing.csproj : error NU1202: Package Splat 4.0.0 is not compatible with netstandard1.1 (.NETStandard,Version=v1.1). Package Splat 4.0.0 supports:

ghuntley added some commits Apr 25, 2018

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented Apr 25, 2018

Is there still a need for a netxoreapp version? What's there that's not in the netstandard version?

@ghuntley ghuntley dismissed stale reviews from olevett and onovotny Apr 25, 2018

scope has increased.

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 25, 2018

netcore is used for unit testing, xamarin workbooks and some folks actually use us serverside. We will need it for anything related to blazor as well.

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 25, 2018

Red build after adjusting targets for UAP. Not sure what's going on here yet.

C:\Program Files\dotnet\sdk\2.1.200-preview-007517\Sdks\Microsoft.NET.Sdk\build\Microsoft.PackageDependencyResolution.targets(167,5): error : Assets file 'C:\code\reactive
ui\ReactiveUI\src\ReactiveUI.Testing\obj\project.assets.json' doesn't have a target for 'UAP,Version=v10.0.14393'. Ensure that restore has run and that you have included '
uap10.0.14393' in the TargetFrameworks for your project. [C:\code\reactiveui\ReactiveUI\src\ReactiveUI.Testing\ReactiveUI.Testing.csproj]

</PropertyGroup>
<PropertyGroup Condition="'$(TargetFramework)' == 'uap10.0'">
<PropertyGroup Condition="'$(TargetFramework)' == 'uap10.0.16299'">
<DefineConstants>$(DefineConstants);NETFX_CORE;XAML;WINDOWS_UWP</DefineConstants>
<TargetPlatformVersion>10.0.14393.0</TargetPlatformVersion>

This comment has been minimized.

@olevett

olevett Apr 25, 2018

Member

We can lose these two I think

@@ -7,4 +7,4 @@
using System.Runtime.CompilerServices;
using System.Windows;

[assembly: InternalsVisibleTo("ReactiveUI.Tests_Net45")]
[assembly: InternalsVisibleTo("ReactiveUI.Tests_Net461")]

This comment has been minimized.

@olevett

olevett Apr 25, 2018

Member

I don't think this is actually used anywhere (other than in the API surface tests, which this should break)

<Name>Windows Desktop Extensions for the UWP</Name>
</SDKReference>
<SDKReference Include="WindowsMobile, Version=10.0.14393.0">
<SDKReference Include="WindowsMobile, Version=10.0.16299.0">

This comment has been minimized.

@onovotny

onovotny Apr 25, 2018

Member

This can go away. There really is no more windows mobile anymore, sadly.

@@ -0,0 +1,5 @@
<Project>

This comment has been minimized.

@onovotny

onovotny Apr 25, 2018

Member

This file isn't needed, can be deleted

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented Apr 25, 2018

@ghuntley I get that, but you don't need a netcoreapp version for that. Put the logic from platforms\netcoreapp2.0 into platforms\netstandard2.0 and you'll be all set.

You should only need a netcoreapp version if there's types you need there that aren't in netstandard. Workbooks, Blazor, et al, will happily use the netstandard version.

Effectively what I'm suggesting is that you don't need a "reference assembly." .NET Standard is a fully featured one with defaults and the platform-specific ones superceed when they're available.

@ghuntley
Copy link
Member

ghuntley left a comment

  1. We used to throw if the end user installed ReactiveUI into their portable library but forgot to install it into their platform application. Now we don't. This is a big change that I'm not comfortable with. It'll be harder for the end user and us to troubleshoot registrations as previously we had certainty that things would explode if the user did something wrong. Now it's silent.
  2. Do we really need to burn netcoreapp? I'd prefer to maintain it as a separate platform (inc any code duplication required to do so) than to change the behavior of 1)
{
public void Register(Action<Func<object>, Type> registerFunction)
{
throw new Exception("You are referencing the Portable version of ReactiveUI in an App. Reference the platform-specific version.");

This comment has been minimized.

@ghuntley

ghuntley Apr 25, 2018

Member
  1. We used to throw if the end user installed ReactiveUI into their portable library but forgot to install it into their platform application. Now we don't. This is a big change that I'm not comfortable with. It'll be harder for the end user and us to troubleshoot registrations as previously we had certainty that things would explode if the user did something wrong. Now it's silent.
  2. Do we really need to burn netcoreapp? I'd prefer to maintain it as a separate platform (inc any code duplication required to do so) than to change the behavior of 1)

This comment has been minimized.

@olevett

olevett Apr 25, 2018

Member

I think I'm happy either way TBH.

From what @onovotny has said, PackageRefs should prevent the problem we used to have from happening (e.g. if you import RxUI in a shared project, and then use that in an app directly, you should get the platform specific version rather than the netstandard version).

I think it's fairly minor either way, so if we're worried I think better to ship with the status quo and then we can merge netcoreapp and netstandard when we're happy the tooling will solve it.

This comment has been minimized.

@onovotny

onovotny Apr 25, 2018

Member

You do not need to reference the right packages anymore because NuGet is better. PackageRef does this always. The only people this bits are people who are using packages.config and mess up.

The Xamarin Templates, and all the other defaults everywhere, use PackageRef now.

This comment has been minimized.

@Qonstrukt

Qonstrukt Apr 26, 2018

Member

The only situation where this might still go wrong is for VS4Mac users. As adding the first package to PackageRef manually is still needed for it to use PackageRefs. By default it still assumes packages.config. I hope this will be fixed in 7.5.

Even if you set up a new solution, it adds required packages in a packages.config file. Annoying...

@ghuntley ghuntley merged commit 4b9d2a4 into develop Apr 30, 2018

2 of 3 checks passed

WIP work in progress – do not merge!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

@ghuntley ghuntley deleted the splat30 branch Apr 30, 2018

@ghuntley ghuntley added housekeeping and removed bug labels May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment