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

React Router Support #407

Merged
merged 12 commits into from Jul 22, 2017

Conversation

Projects
None yet
3 participants
@gunnim
Contributor

gunnim commented May 13, 2017

No description provided.

@gunnim

This comment has been minimized.

Contributor

gunnim commented May 13, 2017

Work is not fully complete but i decided to send the pull request so we could discuss here further.
What's missing are some unit tests, I'm extremely inexperienced in that regard so i'm having some problems implementing tests for methods that call static extension methods.
Was hoping you might have some suggestions on getting around that :)

@Daniel15

This comment has been minimized.

Member

Daniel15 commented May 14, 2017

Thanks, I'll take a look at this soon!

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jun 6, 2017

Have you had a chance to take a look yet? Please let me know if I can do anything to help with the progress :)

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Jun 27, 2017

@gunnim - Sorry for the delay! The past month has been very busy for me 😢 nonetheless, reviewing this is still on my list of things to do. Thanks for your patience!

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jun 27, 2017

Gotcha, glad to hear I'm on the list! :)

@Daniel15

I'm so sorry for taking so long to review this!

Overall, this looks pretty good to me. I left a few comments inline.

Some of the files have very incorrect indentation (eg. lines indented by one space instead of one tab). Could you please fix those too?

build.proj Outdated
@@ -10,8 +10,8 @@ of patent rights can be found in the PATENTS file in the same directory.
<Project ToolsVersion="4.0" DefaultTargets="Build;Test;Package" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<Major>3</Major>
<Minor>0</Minor>
<Build>1</Build>
<Minor>1</Minor>

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Don't worry about bumping this, I'll bump it on release.

build.proj Outdated
@@ -26,8 +26,9 @@ of patent rights can be found in the PATENTS file in the same directory.
<PackageAssemblies Include="React.Core" />
<PackageAssemblies Include="React.MSBuild" />
<PackageAssemblies Include="React.Owin" />
<PackageAssemblies Include="React.Web" />
<PackageAssemblies Include="React.Web.Mvc4" />
<PackageAssemblies Include="React.Router" />

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Indentation looks a bit off here, maybe it's using spaces instead of tabs?

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

Whoops, i went over the .cs files and tabified but i missed this file and some xml. fixed

</namespaces>
</pages>
</system.web.webPages.razor>
</configuration>

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

I don't think this is needed.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

The react router htmlhelper lives in this namespace. I seem to recall being unable to use it if I didn't either have this statement in my web.config or an explicit using statement in file.
You have a similar file in the mvc4 project.

This comment has been minimized.

@Daniel15

Daniel15 Jul 9, 2017

Member

Ah, interesting.

);
}
}
}

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

You can probably just use ReactEnvironment.Current directly rather than wrapping it and rethrowing.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

Sure, that particular block is copied from react.aspnet\htmlhelperextensions.cs.
I imagined you had a reason for doing it there that would also apply here?

This comment has been minimized.

@Daniel15

Daniel15 Jul 9, 2017

Member

In that case, I wanted to show a more specific error message for if ReactJS.NET was misconfigured. I guess that's useful here too though! Maybe ReactEnvironment should have a getter that does that (like ReactEnvironment.GetCurrentOrThrow()) so that the code is not duplicated.

try
{
path = path ?? htmlHelper.ViewContext.HttpContext.Request.Path;
Response = Response ?? htmlHelper.ViewContext.HttpContext.Response;

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Do you actually need the Response argument if it can come from the htmlHelper?

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

No, this was just intended as a convenience and i was hoping it might help for unit testing?

But I guess it might just be confusing, I'll remove it

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

Yeah i'm mocking the response obj in the unit tests, should I be doing this a different way?

This comment has been minimized.

@Daniel15

Daniel15 Jul 9, 2017

Member

i was hoping it might help for unit testing

For unit testing, you should be able to mock the ViewContext property on the IHtmlHelper.


namespace React.Router
{
/// <summary>

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Fix indentation in this file.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

same as above, tabs on my end

[Fact]
public void CreatesIReactComponent()
{
var mocks = new Mocks();

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Fix indentation in this file.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

it's tabbed I swear haha!

@@ -14,7 +14,7 @@ namespace React.Tests.Owin
{
public class EntryAssemblyFileSystemTests
{
[Theory]
[Theory]

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Please fix the indentation.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

It's tabbed. I don't remember but i must have been fixing the indentation since the file shows as changed.

This comment has been minimized.

@Daniel15

Daniel15 Jul 9, 2017

Member

Sorry, looks good to me now, I guess there was a GitHub bug with rendering.

{
public class HtmlHelperExtensionsTest
{
/// <summary>

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Please fix the indentation

}

//[Fact]
//public void EngineIsReturnedToPoolAfterRender()

This comment has been minimized.

@Daniel15

Daniel15 Jul 8, 2017

Member

Remove this if you don't need it.

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

I was hoping for feedback from you on how to fix some of these tests.
I managed to fix
EngineIsReturnedToPoolAfterRender and
EnvironmentShouldGetCalledClientOnly myself but I think i need help with

ReactWithClientOnlyTrueShouldCallRenderHtmlWithTrue and
ReactWithServerOnlyTrueShouldCallRenderHtmlWithTrue
It's a static extension method calling another static extension method. Is there a way to proxy this behavior and have CreateRouterComponent create a Mocked reactcomponent?
Or should I let this and similar tests go?

Also please advise if you feel there should be more tests for some or any parts of this feature

This comment has been minimized.

@Daniel15

Daniel15 Jul 9, 2017

Member

It's a static extension method calling another static extension method. Is there a way to proxy this behavior and have CreateRouterComponent create a Mocked reactcomponent?

Hmm, that's tricky 😕 Static methods can't be mocked. Are you having issues with the test because of that?

This comment has been minimized.

@gunnim

gunnim Jul 9, 2017

Contributor

Booya! Got all of my tests working :)

But my previous statement stands, this is not something I have good experience with so let me know if you think I need more.-.

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Jul 9, 2017

Sorry about the confusing comments around indentation! It looks okay to me now. I guess I was hitting a Github bug - All the code was hard along the left margin, with no indentation at all.

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jul 9, 2017

Hehe no problem, and I did in fact leave some spaces.
Btw vstudio 2017 supports .editorconfig now and moving the file up one folder level set your defaults correctly for me.

Regarding the check failure, should i pull changes from this repo to fix the merge conflict?

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jul 16, 2017

Noooo @Daniel15 !
I can't lose you now, we're so close ;D

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Jul 22, 2017

Looking forward to trying this out - will build it locally and give it a go in one of our projects :)

@Daniel15

Thanks! Let's get this in.

@Daniel15 Daniel15 merged commit 5d1bd9d into reactjs:master Jul 22, 2017

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@gunnim

This comment has been minimized.

Contributor

gunnim commented Jul 22, 2017

Was actually doing exactly the same hehe, got it building but some unit tests are failing on Message: System.IO.FileLoadException : Could not load file or assembly 'JavaScriptEngineSwitcher.Core, Version=2.4.0.0, Culture=neutral, PublicKeyToken=c608b2a8cc9e4472' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

I actually just finished pulling your 3.1.0 release building from source and I get the same error there

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Jul 22, 2017

Message: System.IO.FileLoadException : Could not load file or assembly 'JavaScriptEngineSwitcher.Core, Version=2.4.0.0, Culture=neutral, PublicKeyToken=c608b2a8cc9e4472' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference.

Try doing a clean build and see if that works. Usually this happens when you have an old assembly in the bin directory. "The located assembly's manifest definition does not match the assembly reference" uisually means that the version number in the bin directory differs from the version number that the app actually references.

It looks like the AppVeyor build is working: https://ci.appveyor.com/project/Daniel15/react-net/build/269

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jul 22, 2017

So what did i did after trying to get the merge of branches working was...
I cloned 3.1.0 straight from this repo fresh, ran dev-build.bat, then build from vstudio on the tests and then ran them.... same error

@gunnim

This comment has been minimized.

Contributor

gunnim commented Jul 29, 2017

@Daniel15
This happens when you try to run the unit tests.
The issue seems to have to do with the fact that the v8 engine references javascriptengineswitcher.core v. 2.4.0 while the rest of the solution references 2.4.9
I also tried out upgrading all the js engine related packages but then the issue became that jspool was referencing v. 2.4.9 after i had upgraded to 2.4.10

This issue should be easily reproducible since I verified by doing a fresh clone of version 3.1.0.

Problem goes away if i manually specify a binding redirect.
ASP.NET projects are clever enough to automagically add a module binding redirect for the conflicting versions however.

image

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Nov 8, 2017

The build still seems to be working fine for me. Released this today as version 3.2. Sorry for the delay. Thanks!

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