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

Simplify html helper function #438

Merged
merged 3 commits into from Nov 20, 2017

Conversation

Projects
None yet
3 participants
@gunnim
Contributor

gunnim commented Sep 17, 2017

I think I made a mistake in the API naming. Overly detailed name which will just overly burden callers.
Would love it if this would make it in before the feature gets released :)

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Oct 11, 2017

Looks like a test failure?

  Failed   React.Tests.Router.HtmlHelperExtensionsTest.ShouldRedirectPermanent
EXEC : error Message:  [C:\projects\react-net\build.proj]
   System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at React.Router.HtmlHelperExtensions.ReactRouter[T](HtmlHelper htmlHelper, String componentName, T props, String path, String htmlTag, String containerId, Boolean clientOnly, Boolean serverOnly, String containerClass, Action`2 contextHandler)
     at React.Tests.Router.HtmlHelperExtensionsTest.ShouldRedirectPermanent() in C:\projects\react-net\tests\React.Tests\Router\HtmlHelperExtensionsTest.cs:line 225
@gunnim

This comment has been minimized.

Contributor

gunnim commented Oct 11, 2017

This is beyond weird... the test fails intermittently... with no obvious cause. Seems like an issue more with the test than the code itself but i'm gonna have to take a better look

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Oct 12, 2017

Sorry for the delay in replying... I've been super busy recently. I'll take a look at this soon :)

@gunnim

This comment has been minimized.

Contributor

gunnim commented Oct 12, 2017

I tried my best but all i came up with is.
The bug never happens when i run the tests in visual studio, only when I run the tests using
dotnet test --configuration Debug --no-build tests/React.Tests/React.Tests.csproj

with some console writeline statements i managed to track it to line 97 in HtmlHelperExtensions
if (executionResult.Context?.status != null)

Intermittently during various tests and when the exception happens, calling ToString on this executionResult obj produces the following
MockReact.Router.ExecutionResult:3821

I can't see any way for that to happen, let alone intermittently...
this is also the string value of the object when the exception happens

@gunnim

This comment has been minimized.

Contributor

gunnim commented Nov 15, 2017

I understand if this won't get merged.
But in case it might, this code has backwards compatibility, marking the originally named method obsolete and has said method simply call the newer shorter name.

@Daniel15 Daniel15 merged commit 546aecd into reactjs:master Nov 20, 2017

1 check passed

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

This comment has been minimized.

Member

Daniel15 commented Nov 20, 2017

Thanks! Sorry I didn't merge this before the release :(

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