Fix 361

[Aledever4i]

Hello, I'm very tired of your chatting about core runtime ( #361 ) as result did this commit.

Please check it and give your opinion.

With sincerely,
Igor Sukhov

[jzabroski]

Hi Igor,

Apologies for not getting to this sooner - I caught COVID and it has been a long recovery.

Can you explain why this fixes the issue?

[Aledever4i]

I want to help you maintain this framework, and this is one step for release 2.0 version. We have #360 issue about <FrameworkReference Include="Microsoft.AspNetCore.App" />, because if you change it to <PackageReference Include="Microsoft.AspNetCore.Razor.Runtime" Version="2.2.0" />, you'll take error from compiler (CS1061 error).

What i did is separete tests for web and desktop (base logic). Web uses Microsoft.NET.Sdk.Web , desktop other library.

[tebeco]

this does not look like a good idea at all
trying to get portability at the price of depending on Obsolete / possibly CVE affected package

Probably all consumer having CVE check would have to opt out from RazorLight

At the same time introducing possible crash for consumer using AspNetCore on netcoreapp3.x / net5.0 / net6.0 if they actively needs new API since 3.x from Types inside the following assemblies:

Microsoft.AspNetCore.Html.Abstractions
Microsoft.AspNetCore.Razor
Microsoft.AspNetCore.Razor.Design
Microsoft.AspNetCore.Razor.Runtime

introducing this change would then also make lots of people jump out the boat for that reason.
if you want to hard pin on 2.2.0 I suggest you create a TFM for netcoreapp2.1 or netcoreapp2.2 and you stick to that

The cost of FrameworkReference is, unless I'm mistaken, just space disk on the output artifact, instead of creating possible MissingMethodException here.

The alternative is to split this repo is RazorLight and RazorLight.WithoutAspNetCore something like that

[tebeco]

redundant test on != and == here ?

[tebeco]

aside questionning the Optimize>false for Release here, that could be simplified to:

Suggested change

	<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
	<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	</PropertyGroup>
	<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">
	<Optimize>false</Optimize>
	<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	</PropertyGroup>
	<PropertyGroup>
	<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	</PropertyGroup>
	<PropertyGroup Condition="'$(Configuration)'=='Release'">
	<Optimize>false</Optimize>
	</PropertyGroup>

[tebeco]

Same as earlier, this should be imported by the FrameworkReference as you're already using the Web.Sdk
Forcing it down would just cause issue when you'll call any new API since 3.x / 5.x / 6.x

Even if it's force testing it would give the idea that it work, while it will actually depends on consumers call and possibly hit a MissingMethodException

[tebeco]

unless this project is a shell to be ran by an [WebApplicationFactory](https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-5.0) i would recommend moving back to standard SDK, and add a FrameworkReference into that project.

My (possibly wrong) assymption here is that the test project is relying on the FrameworkReference that the Microsoft.NET.Sdk.Web does for consumer. FrameworkReference is a better approach is all you need is the Reference out of it

[tebeco]

Suggested change

	<PropertyGroup Condition="'$(OS)' == 'Windows_NT'">
	<TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
	</PropertyGroup>
	<PropertyGroup Condition="'$(OS)' != 'Windows_NT'">
	<TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
	</PropertyGroup>
	<PropertyGroup>
	<TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
	</PropertyGroup>

[tebeco]

can you link the doc page about what is MdTargetDir ? You got me curious on that one

[jzabroski]

Closing this, as #467 effectively sidesteps this problem entirely by dropping .NET Framework support. CC @toddams