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

Making React.AspNet compatible with ASP.NET Core RC2 #271

Merged
merged 6 commits into from May 22, 2016

Conversation

Projects
None yet
5 participants
@ShikiGami
Copy link
Contributor

ShikiGami commented May 21, 2016

  • Changes the names of assemblies from ASP.NET 5 RC1 to ASP.NET Core RC2
  • Changes to the new structure of project.json and xproj
  • All old DNX commands were replaced by the new dotnet cli.
  • IApplicationEnvironment was deprecated, and all its information was reallocated to IHostingEnvironment .
  • StaticFileMiddleware now requires an IOption<StaticFileOptions> instead of a plain StaticFileOptions.
  • IHttpContextAccessor isn't set any longer by default. Since we don't need HttpContext.RequestServices in order to get the PerRequestRegistrations service, and we can get it directly from IApplicationBuilder.ApplicationServices, I just deleted solving the service all together.

Fix issue #268 and #269

ShikiGami added some commits May 18, 2016

Building on ASP.NET Core RC2
* It's able to build using dev-build
* It's able to run IIS Express
* It fails at runtime, React.AspNet cannot resolve React.Core dependency on runtime.
* The output folder for React.AspNet has become more messy, not sure if this is something that can be solved
* It is not creating the nupkg for React.AspNet
@@ -2,8 +2,6 @@ version: '{build}'
os: Visual Studio 2015
install:
- set PATH=%ProgramFiles(x86)%\MSBuild\14.0\Bin;%PATH%
- dnvm update-self
- dnvm install 1.0.0-rc1-update1

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Should this run the equivalent dotnet command to ensure RC2 is installed on the build machine?

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

As far as I know it no longer works that way in RC2. There is no equivalent to dnvm in the new toolset. If you have .NET Core RC2 installed already, you can just get the ASP.NET Core packages using NuGet.
That was one of the biggest changes from RC1 to RC2, and it was done in order to make all .NET Core environments consistent with each other.

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Thanks for the info 👍

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Looks like they've got an install script for CI servers (see https://dotnet.github.io/docs/core-concepts/dnx-migration.html). However, their docs for AppVeyor just say "TODO" (https://dotnet.github.io/docs/core-concepts/core-sdk/cli/using-ci-with-cli.html). It's fine to ignore that for now if the AppVeyor build actually works with no changes.

_options.Extensions
)
},
Options.Create(new StaticFileOptions

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Could you please adjust the spacing here? It should be using tabs rather than spaces. I though there was an Editorconfig file in the repo but it looks like I forgot to commit it.

[assembly: AssemblyTitle("React.AspNet")]
[assembly: AssemblyDescription("ReactJS and Babel tools for ASP.NET Core, including ASP.NET Core MVC")]
[assembly: ComVisible(false)]
[assembly: Guid("f0e355fc-a89f-4fd6-b171-1ef03c838a67")]

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Do we still need this now that you reverted it from being a .csproj?

<PropertyGroup Label="Globals">
<ProjectGuid>a7acdb56-5e43-40a6-92c9-2c52228e6074</ProjectGuid>
<ProjectGuid>f0e355fc-a89f-4fd6-b171-1ef03c838a67</ProjectGuid>

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Keep the GUID the same? I don't see a reason to change it :)

</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\React.Core\React.Core.csproj" />

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Is the project reference no longer needed?

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

It really isn't necessary now. What could be done to make a project reference, is to make the reference inside of project.json, where instead of declaring the version, you target the project.

@@ -40,7 +40,6 @@ public static class ReactBuilderExtensions
// Register IApplicationEnvironment in our dependency injection container
// Ideally this would be in AddReact(IServiceCollection) but we can't
// access IApplicationEnvironment there.
React.AssemblyRegistration.Container.Register(app.ApplicationServices.GetRequiredService<IApplicationEnvironment>());

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Remove the comment too, please.

"configurations": {
"Debug": {
"buildOptions": {
"define": [ "DEBUG", "TRACE", "ASPNET5" ]

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

I wonder if we should rename the ASPNET5 define too. It's referenced in a few source files via #ifdef ASPNET5

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

I looked at the files, but I couldn't find any reference to ASPNET5.

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Ah, good point. Setting preprocess directives was actually broken for ASP.NET 5 projects, so all the conditions instead check for LEGACYASPNET and assume a ASP.NET 5 environment if not set (eg.

). I guess you can rename this without changing anything then :)

}
},
"scripts": {
"postcompile": [ "dotnet pack --output ../../bin/React.AspNet/ --no-build --version-suffix %project:Version% project.json" ]

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

This is totally fine, I wonder if it should be done in build.proj along with all the other NuGet packages, to keep everything consistent.

Thanks for figuring this out, btw :)

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

I was thinking exactly the same thing, I'm going to try it on build.proj

<assemblyIdentity name="JavaScriptEngineSwitcher.V8" publicKeyToken="C608B2A8CC9E4472" culture="neutral"/>
<bindingRedirect oldVersion="0.0.0.0-1.5.0.0" newVersion="1.5.0.0"/>
<assemblyIdentity name="JavaScriptEngineSwitcher.V8" publicKeyToken="C608B2A8CC9E4472" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-1.5.0.0" newVersion="1.5.0.0" />

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Revert all these changes please, since they're just changing whitespace.

@@ -0,0 +1,24 @@
using System;

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Add the copyright header

// Add the platform handler to the request pipeline.
app.UseIISPlatformHandler();

{

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Use tabs rather than spaces

@@ -52,7 +52,8 @@ of patent rights can be found in the PATENTS file in the same directory.
var contents = File.ReadAllText(path);
var newContents = versionRegex.Replace(
contents,
string.Format("\"version\": \"{0}\"", Version)
string.Format("\"version\": \"{0}\"", Version),
1

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

What is this for?

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

This is in order to just replace the first instance of version numbering in the .json files. This is done because now in RC2 there are a couple more places where you need to use version number, like when declaring CLI tools.

@@ -3,7 +3,7 @@
"wrap"
],
"sdk": {
"version": "1.0.0-rc1-update1"
"version": "1.0.0-preview1-002702"

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Is this a final release? Should it be rc2 rather than preview1?

This comment has been minimized.

@ShikiGami

ShikiGami May 22, 2016

Contributor

The assemblies are RC2, but the SDK + tooling are preview 1. By the time it hits 1.0 the SDK + tooling will be preview 2, and version 1.0 of SDK + tooling is going to be released latter.
This is because DNX was completely deprecated, and the new CLI took over, but since it is completely new, it still isn't as stable to be called a Release Candidate.

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 22, 2016

Thanks so much for working on this! Just a few small comments. Please use tabs rather than spaces in .cs files 😄

Please leave a comment once you update the pull request, unfortunately Github doesn't send notifications for updates to pull requests.

Moved React.AspNet package creation to build.proj and other fixes
* Reverted unnecesary white space
* Deleted unnecesary comment
* Added missing copyright notice
* Fixed indentation
* Changed target of local dependencies to project
* Reverted React.AspNet's old GUID
* Deleted React.AspNet's AssemblyInfo.cs
* Changing ASPNET5 preprosesor directive name to ASPNETCORE
* Fixed project.json so that the NuGet package would show the correct info
* Fixed dotnet pack script so that it doesn't create empty packages
@ShikiGami

This comment has been minimized.

Copy link
Contributor

ShikiGami commented May 22, 2016

Just committed the fixes you asked for.
I also realized that dotnet pack was creating empty packages because apparently it cannot read the .xproj and therefore it doesn't know where is the proper outpath for the project's binaries and objects, so I also fixed the script and confirmed it was creating the proper packages.

"MsieJavaScriptEngine": "1.7.0"
"MsieJavaScriptEngine": "1.7.0",
"React.Core": {
"target": "project"

This comment has been minimized.

@Daniel15

Daniel15 May 22, 2016

Member

Oh cool, I didn't know about being able to target projects like this! Is there documentation on it?

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 22, 2016

Thank you so much, this is a fantastic pull request. I've got a few of my own projects to upgrade from RC1 to RC2, and this PR is a good example that I can follow for the other projects 😄

@Daniel15 Daniel15 merged commit fe6f54f into reactjs:master May 22, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 23, 2016

Hey @ShikiGami, I'm having trouble building this on my computer:

  Errors in C:\src\ReactJS.NET\src\React.AspNet\React.AspNet.xproj
      React.AspNet 2.3.0 is not compatible with .NETFramework,Version=v4.5.1.
      Some packages are not compatible with .NETFramework,Version=v4.5.1.

However, I see that it's working fine on AppVeyor, so I guess it should be working. Any ideas?

@ShikiGami

This comment has been minimized.

Copy link
Contributor

ShikiGami commented May 23, 2016

Some packages are not compatible with .NETFramework,Version=v4.5.1.

It isn't telling you which packages aren't compatible?

@wassim-azirar

This comment has been minimized.

Copy link

wassim-azirar commented May 23, 2016

@Daniel15 Does it work on your computer ?
When will this PR be merged ?

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 24, 2016

Strangely I can't replicate that issue any more. ¯_(ツ)_/¯

@wassim-azirar - It's already been merged, you can use the packages from the development server if you like (details at http://reactjs.net/getting-started/download.html#development-builds)

@wassim-azirar

This comment has been minimized.

Copy link

wassim-azirar commented May 24, 2016

I followed the instructions but I'm still having the same error :(
rc2

@ShikiGami

This comment has been minimized.

Copy link
Contributor

ShikiGami commented May 24, 2016

@wassim-azirar
It isn't compatible with .NETCoreApp, you need to use the full .NET Framework for it.
In frameworks replace netcoreapp1.0 with net451

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 24, 2016

Yeah, not all dependencies are available for .NET Core (eg. currently there's no JS engine that runs on .NET Core) so you need to use the full .NET Framework.

);
}
var registrations = requestServices.GetService<PerRequestRegistrations>();
var registrations = _appServiceProvider.GetService<PerRequestRegistrations>();

This comment has been minimized.

@Daniel15

Daniel15 May 24, 2016

Member

This change was incorrect, it wasn't handling per-request singletons properly and was instead just using a single instance for the entire app. I fixed it in a4992da 👍

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 27, 2016

This has been included in the 2.4 release (http://reactjs.net/2016/05/2.4.0-release.html). Thanks for your contribution!

@pauldotknopf

This comment has been minimized.

Copy link

pauldotknopf commented Jun 6, 2016

I updated VroomJs to support .NET Core.

https://github.com/pauldotknopf/vroomjs-core

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented Jun 23, 2016

@pauldotknopf - That's great news! I should probably update JsPool to have a .NET Core build, then we should be able to get ReactJS.NET running on .NET Core :)

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