reactjs / React.NET

.NET library for JSX compilation and server-side rendering of React components
https://reactjs.net/
MIT License
2.29k stars 937 forks source link

How to show error page for server-side script errors? #786

Closed Reonekot closed 5 years ago

Reonekot commented 5 years ago

I my pursuit to validate this project as a good way forward for us in introducing React server-side rendering “piecemeal”, I ran into a few issues around error handling.

In a ASP.NET Core view when calling @Html.React() (or other HtmlExtention methods) and the JS has errors, the JavaScriptEngineFactory will throw an error - which is to be expected. For V8 the error is quite clear, though for ChakraCore not so much – but I assume that’s a problem upstream.

My problem is that this result in an aborted HTTP 200 response instead of 500 error response and that the exception is somehow interfering with the exception handling middleware. This results in another exception with the message "The response has already started, the error page middleware will not be executed.”. This is problematic for two reasons – it makes it hard for developers to see the real exception since they need to find the logs and in production it means the customers will just see a partially rendered page instead of “a pretty error page”.

I tried looking into the code, but I can’t really see why this is happening or what has started writing to the output at this point, so I hope someone with more experience with this has some clues. If I create view with the following:

@using React.AspNet
@{ Layout = null; }
@Html.React("ReactTestComponent", new { });

This will just return an empty page, instead of a developer exception page. (Just to be clear, ReactTestComponent has invalid JS, so it is expected to throw an error.) For reference, I have disabled LoadBabel+LoadReact and using this setup method for this test:

app.UseReact(config =>
{
    config
        .SetLoadBabel(false)
        .SetLoadReact(false)
        .AddScriptWithoutTransform("~/scripts/reactComponents/reactComponents.js");
    config.UseServerSideRendering = true;
});

Normally if I throw an error in a view (fx. Replacing the Html.React() with “var x1 = 0; var x2 = 1 / x1;” or simply a throw statement) this works fine and results in a 500 error.

I tried following the code in the repo and I can’t find what might be causing this – if I follow it correctly, it goes something like: HtmlHelperExtensions.React() (https://github.com/reactjs/React.NET/blob/master/src/React.AspNet/HtmlHelperExtensions.cs#L69) -> Environment.CreateComponent() (https://github.com/reactjs/React.NET/blob/master/src/React.Core/ReactEnvironment.cs#L276) -> -> which tries to create an Engine and ends in JavaScriptEngineFactory.LoadUserScripts() (https://github.com/reactjs/React.NET/blob/master/src/React.Core/JavaScriptEngineFactory.cs#L172) which throws.

Now this all seems fine to me, but I can’t figure out what has written to the output at this point. Iit looks like nothing has accessed the “stringwriter” up until this point in HtmlHelperExtensions.React(). ☹ So I’m not sure why, apparently, throwing from JavaScriptEngineFactory.LoadUserScripts() doesn’t give the same result as if I put a “throw new Exception(“test”);” instead of @Html.React() in the view.

Any insights to what might be going on and hopefully ideas to fix this behavior?

Taritsyn commented 5 years ago

For V8 the error is quite clear, though for ChakraCore not so much – but I assume that’s a problem upstream.

@Reonekot You can give an example of differences in error messages?

In the JavaScript Engine Switcher version 3.X was done work on the unification of error messages.

Compilation error

ChakraCore
==========

SyntaxError: Unexpected identifier after numeric literal
   at declinationOfSeconds.js:12:23 ->          caseIndex = number % 1O < 5 ? number % 10 : 5;

V8
==

SyntaxError: Invalid or unexpected token
   at declinationOfSeconds.js:12:24 ->          caseIndex = number % 1O < 5 ? number % 10 : 5;

Runtime error

ChakraCore
==========

TypeError: Unable to get property 'Ч' of undefined or null reference
   at transliterate (russian-translit.js:929:4) ->                      newCharValue = typeof charMapping[charValue] !== 'undefined' ?
   at Global code (Script Document:1:1)

V8
==

TypeError: Cannot read property 'Ч' of undefined
   at transliterate (russian-translit.js:929:37) ->                     newCharValue = typeof charMapping[charValue] !== 'undefined' ?
   at Script Document:1:1
dustinsoftware commented 5 years ago

Do you see the same problem in the .NET Core sample in this repo?

It does sound like a bug if there’s a problem loading user scripts.. that should return a 500.

Can you also list the package versions? There as an issue template that appears to have been dropped in your bug report.

Reonekot commented 5 years ago

@Taritsyn - Sorry about that, apparently I can't reproduce this again. There must be something else interfering with the errors in our project, which lead my to the wrong conclusion. I thought I remembered what had triggered it (forgetting "this." in "this.props.xxx"), but reversing to have that JS error gives pretty much the same result on ChakraCore and V8, as you said.

ChakraCore "React.Exceptions.ReactServerRenderingException: Error while rendering \"ReactTestComponent2\" to \"react_0HLMA3PM7D23Q\": ReferenceError: 'props' is not defined\r\n at render (reactComponents.js:28698:7) -> throw err;\r\n at processChild (reactComponents.js:3283:5)\r\n at resolve (reactComponents.js:3136:5)\r\n at render (reactComponents.js:3526:7)\r\n at read (reactComponents.js:3485:11)\r\n at renderToString (reactComponents.js:3970:5)\r\n at Global code (Script Document [2]:1:1) ---> JavaScriptEngineSwitcher.Core.JsRuntimeException: ReferenceError: 'props' is not defined\r\n at render (reactComponents.js:28698:7) -> throw err;\r\n at processChild (reactComponents.js:3283:5)\r\n at resolve (reactComponents.js:3136:5)\r\n at render (reactComponents.js:3526:7)\r\n at read (reactComponents.js:3485:11)\r\n at renderToString (reactComponents.js:3970:5)\r\n at Global code (Script Document [2]:1:1) ---> JavaScriptEngineSwitcher.ChakraCore.JsRt.JsScriptException: Script threw an exception.\r\n at JavaScriptEngineSwitcher.ChakraCore.JsRt.JsErrorHelpers.ThrowIfError(JsErrorCode error) ...

V8 "React.Exceptions.ReactServerRenderingException: Error while rendering \"ReactTestComponent2\" to \"react_0HLMA3SGP7P7N\": ReferenceError: props is not defined\r\n at render (reactComponents.js:28698:119) -> throw err;\r\n at processChild (reactComponents.js:3283:18)\r\n at resolve (reactComponents.js:3136:5)\r\n at render (reactComponents.js:3526:22)\r\n at read (reactComponents.js:3485:29)\r\n at renderToString (reactComponents.js:3970:27)\r\n at Script Document [3]:1:16 ---> JavaScriptEngineSwitcher.Core.JsRuntimeException: ReferenceError: props is not defined\r\n at render (reactComponents.js:28698:119) -> throw err;\r\n at processChild (reactComponents.js:3283:18)\r\n at resolve (reactComponents.js:3136:5)\r\n at render (reactComponents.js:3526:22)\r\n at read (reactComponents.js:3485:29)\r\n at renderToString (reactComponents.js:3970:27)\r\n at Script Document [3]:1:16 ---> Microsoft.ClearScript.ScriptEngineException: ReferenceError: props is not defined\r\n at V8Exception.ThrowScriptEngineException(V8Exception* ) ...

So I'm the error has probably been something else and it was fixed at the same time I tried changing from ChakraCore to V8. I used 3.0.7 of ChakraCore at the time, but have since updated to 3.0.9.

Reonekot commented 5 years ago

dustinsoftware : It was my plan to link to https://github.com/reactjs/React.NET/issues/769, which includes the version template stuff, but forgot it in the end - sorry about that. Only difference, is that ChakraCore is updated to newest version, but else it should all be up-to-date.

Anyway, I tried reproducing from the .NET Core webpack example project today and that project work as expected (unfortunately for me...). I tried changing a few things in that project, so it matches my setup, basically disabling Bable, so config for both projects is just .SetLoadBabel(false).SetLoadReact(false).AddScriptWithoutTransform() + adding razor view compilation, but it still works as expected (showing the developer exception page) in that project.

So I tried minimizing middleware setup in our project, so now it is basically just:

app.UseDeveloperExceptionPage();
app.UseReact(...)
app.UseSession();
app.UseMvc(ConfigureRoutes);

And it still doesn't work on our project :( Very frustrating stuff...

I'll see if I can get to the bottom of this Monday/Tuesday. At the moment I can't really think of much else difference besides we are running .NET Core on full framework 4.7.2 and not .NET Core runtime and that we are hosting on IIS.

As said, my test view has no code or layout and if I just "throw new Exception("");" it works fine, so I'm a bit at a loss as to why the same doesn't happen calling React() or ReactInitJavaScript().

Reonekot commented 5 years ago

I finally managed to isolate the issue and reproduce it in the React.Sample.Webpack.CoreMvc project.

From a clean checkout I did the following:

  1. Run dev-build.bat
  2. Make "wwwroot/dist/components.js" contain invalid JS (I just insert "test" on the second line)
  3. In Home/Index.cstml either: a. Remove the call to Html.ReactRouter() (which means the call to ReactInitJavaScript() is the only thing in there) b. or call Html.React() instead of ReactRouter()
  4. In Home/Index.cshtml I added a "The end" on the last line just to get some output if it "fails the wrong way"
  5. Start the project (dotnet run bin\Release\netcoreapp2.1\React.Sample.Webpack.CoreMvc.dll)

The result of the above, for me, is a HTTP 200 response and not the developer exception page.

I took a look at the difference between React() and ReactRoute() and I can see one is "lazy" (React()) and the other isn't and apparently this lazyness of writing to the writer triggers this behavior. So adding this code to the view "fixes" the issues:

@{
    var output = Html.ReactInitJavaScript();
    using (var writer = new System.IO.StringWriter())
    {
        output.WriteTo(writer, System.Text.Encodings.Web.HtmlEncoder.Default);
    }
}
@output

I also tried adding the above hack to our project, wrapping Html.React() in the above call to WriteTo() and now it throws as expected.

So basically the issues is with the ActionHtmlString wrapper at https://github.com/reactjs/React.NET/blob/master/src/React.AspNet/HtmlHelperExtensions.cs#L69 as I see it. One way to fix it, I think, for React() would be to pull out Environment.CreateComponent() so it will throw at that point. I'm not sure about how to fix ReactInitJavaScript() though, besides adding an explicit call that checks if the Engine is initialized or simple requiring that either React/ReactRouter() must allways be called (which might already be a requirement - but always nice to fail "correctly" even when misused.)

dustinsoftware commented 5 years ago

Thanks for digging... I’ll take a look

On Tue, 30 Apr 2019 at 03:48, Toke Noer notifications@github.com wrote:

I finally managed to isolate the issue and reproduce it in the React.Sample.Webpack.CoreMvc project.

From a clean checkout I did the following:

  1. Run dev-build.bat
  2. Make "wwwroot/dist/components.js" contain invalid JS (I just insert "test" on the second line)
  3. In Home/Index.cstml either: a. Remove the call to Html.ReactRouter() (which means the call to ReactInitJavaScript() is the only thing in there) b. or call Html.React() instead of ReactRouter()
  4. In Home/Index.cshtml I added a "The end" on the last line just to get some output if it "fails the wrong way"
  5. Start the project (dotnet run bin\Release\netcoreapp2.1\React.Sample.Webpack.CoreMvc.dll)

The result of the above, for me, is a HTTP 200 response and not the developer exception page.

I took a look at the difference between React() and ReactRoute() and I can see one is "lazy" (React()) and the other isn't and apparently this lazyness of writing to the writer triggers this behavior. So adding this code to the view "fixes" the issues:

@{ var output = Html.ReactInitJavaScript(); using (var writer = new System.IO.StringWriter()) { output.WriteTo(writer, System.Text.Encodings.Web.HtmlEncoder.Default); } } @output

I also tried adding the above hack to our project, wrapping Html.React() in the above call to WriteTo() and now it throws as expected.

So basically the issues is with the ActionHtmlString wrapper at https://github.com/reactjs/React.NET/blob/master/src/React.AspNet/HtmlHelperExtensions.cs#L69 as I see it. One way to fix it, I think, for React() would be to pull out Environment.CreateComponent() so it will throw at that point. I'm not sure about how to fix ReactInitJavaScript() though, besides adding an explicit call that checks if the Engine is initialized or simple requiring that either React/ReactRouter() must allways be called (which might already be a requirement - but always nice to fail "correctly" even when misused.)

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/reactjs/React.NET/issues/786#issuecomment-487891764, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHGCFQEMQMG6XCXOMXSZTDPTAIYRANCNFSM4HINQTFQ .

dustinsoftware commented 5 years ago

Fixed by #793

Reonekot commented 5 years ago

Thanks !

FYI, if your are interested, I did some quick benchmark of it locally, comparing ChakraCore with pure MVC. This is with our full middleware pipeline setup (which does user lookup etc.), but without extra data fetch/logic in the Controller and a single very simple React component and my conclusion is that it looks quite good to me. 391 request/sec for React, 427 for the same in MVC with just a RenderPartialAsync() with the same markup for a single 60 sec run example. Median responsetime for this test was 72 ms. vs. 75 ms., so if this scales that is very good compared to what we have seen on our other solutions using NodeJS.

Ed: Btw. the above test was with before #793, so with a "hack" to write output via: "reactOutput.writeTo(this.ViewContext.Writer, HtmlEncoder.Default)"