Throw unhandled exceptions during prerendering #8616

rynowak · 2019-03-18T03:17:06Z

Currently exceptions thrown during prerendering are simply logged. This
change uses the existing unhandled exception mechanism of the
renderer/circuit to throw these. The result is that the developer
exception page just works for prerendering.

rynowak · 2019-03-18T03:19:35Z

src/Mvc/test/Mvc.FunctionalTests/ComponentRenderingFunctionalTests.cs

@@ -17,18 +19,17 @@ public class ComponentRenderingFunctionalTests : IClassFixture<MvcTestFixture<Ba
        public ComponentRenderingFunctionalTests(MvcTestFixture<BasicWebSite.StartupWithoutEndpointRouting> fixture)
        {
            Factory = fixture;
-            Client = Client ?? CreateClient(fixture);


The paradigm used in these tests was broken now that we report errors. The FetchData component is part of all of these tests, and when used with prerendering it was throwing an exception because the service configuration wasn't correct.

I can't help feeling like this is all more complicated than necessary. Using httpclient to call your own API isn't something we'd normally tell users to do.

you can simply do Client ?? CreateClient(fixture.WithWebHostBuilder(b => b.ConfigureServices(s => s.AddRazorComponents())) or extend the MVC fixture and configure it there. Both work. I usually do the WithWebHostBuilder because I'm cheap and I don't bother with inheritance, but this is something you can do a nested class of the test and that's totally fine.

rynowak · 2019-03-18T03:19:57Z

src/Mvc/test/WebSites/BasicWebSite/RazorComponents/Throws.razor

+    protected override async Task OnInitAsync()
+    {
+        await base.OnInitAsync();
+        throw new InvalidTimeZoneException("test");


One of my favourites for testing, I've never seen this in the wild.

rynowak · 2019-03-18T03:21:57Z

src/Mvc/test/WebSites/BasicWebSite/RazorComponents/Throws.razor

@@ -0,0 +1,10 @@
+@page "/components/throws"
+


Note that I didn't include a test for the case where a component throws during construction/DI/parameters. That results in #8190

javiercn · 2019-03-18T08:50:48Z

I already have tests and a fix for this on the components relayering PR, btw

src/Components/Server/src/Circuits/CircuitPrerenderer.cs

SteveSandersonMS

Looks solid

rynowak · 2019-03-19T18:49:03Z

@SteveSandersonMS - could you take a second look at this?

I have more that I want to follow up on but I will do that in a separate PR.

rynowak · 2019-03-19T18:49:51Z

src/Components/Components/src/EventCallbackFactoryBinderExtensions.cs

@@ -11,74 +11,263 @@ namespace Microsoft.AspNetCore.Components
    /// </summary>
    public static class EventCallbackFactoryBinderExtensions
    {
+        private delegate bool BindConverter<T>(object obj, out T value);


I reworked all of this logic to avoid throwing exceptions where possible. Now that we're handling the exceptions here it just makes sense to avoid the noise of throwing them in the first place. Now all of this code is tryparse where possible.

rynowak · 2019-03-19T21:29:14Z

src/Components/Components/src/EventCallbackFactoryBinderExtensions.cs

-            return null;
-        };
+            value = converted;
+            return true;


The difference between nullable and non-nullable:

Nullable treats empty string as null and will bind successfully

non-nullable fails to parse empty string, and will retain the previous value.

It's problematic to convert an empty value to default for non-nullable. It will check the value of the text field to 0 instead of "".

SteveSandersonMS

Looks extra-solid now! I'm glad we're finally filling in some of the details of these scenarios.

Fixes: #8609 Currently exceptions thrown during prerendering are simply logged. This change uses the existing *unhandled exception* mechanism of the renderer/circuit to throw these. The result is that the developer exception page just works for prerendering.

javiercn · 2019-03-20T19:09:20Z

src/Components/Server/src/Circuits/CircuitPrerenderer.cs

@@ -26,8 +28,12 @@ public async Task<IEnumerable<string>> PrerenderComponentAsync(ComponentPrerende
                GetFullUri(context.Request),
                GetFullBaseUri(context.Request));

+            // We don't need to unsubscribe because the circuit host object is scoped to this call.
+            circuitHost.UnhandledException += CircuitHost_UnhandledException;


On second look, I don't think this is correct. The circuit will live longer than the request and when the signalr connection gets stablished you don't want to have this handler throwing.

If you take a look at how i'm doing this on my PR
https://github.com/aspnet/AspNetCore/pull/7954/files#diff-d33c04a2c673816a9a5c6e959a8e5270R91

I'm specifically basing it on whether I'm prerrendering or not.

You can keep this if you want to, but I'm gonna have to change it on my PR so it just adds work to me :(

I'll leave it up to yo.

I don't understand this, or how to do extra work to support something that isn't implemented yet, so I'm going to go ahead and merge this 😆

rynowak requested a review from javiercn March 18, 2019 03:17

rynowak requested a review from SteveSandersonMS as a code owner March 18, 2019 03:17

rynowak commented Mar 18, 2019

View reviewed changes

javiercn approved these changes Mar 18, 2019

View reviewed changes

javiercn reviewed Mar 18, 2019

View reviewed changes

src/Components/Server/src/Circuits/CircuitPrerenderer.cs Outdated Show resolved Hide resolved

rynowak force-pushed the rynowak/prerendering branch from 31da0de to cb146ab Compare March 18, 2019 15:03

SteveSandersonMS approved these changes Mar 18, 2019

View reviewed changes

rynowak force-pushed the rynowak/prerendering-error-throwing branch 2 times, most recently from 7780b9e to 767e368 Compare March 18, 2019 17:19

rynowak requested review from jkotalik and Tratcher as code owners March 18, 2019 17:19

rynowak changed the base branch from rynowak/prerendering to master March 18, 2019 17:19

Tratcher removed request for Tratcher and jkotalik March 18, 2019 17:28

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 18, 2019

rynowak force-pushed the rynowak/prerendering-error-throwing branch 2 times, most recently from 1def9b7 to 862daf0 Compare March 19, 2019 18:48

rynowak commented Mar 19, 2019

View reviewed changes

rynowak force-pushed the rynowak/prerendering-error-throwing branch from 862daf0 to ca14bc7 Compare March 19, 2019 21:34

SteveSandersonMS self-requested a review March 20, 2019 00:12

SteveSandersonMS approved these changes Mar 20, 2019

View reviewed changes

rynowak force-pushed the rynowak/prerendering-error-throwing branch from ca14bc7 to 6fdde36 Compare March 20, 2019 15:00

javiercn reviewed Mar 20, 2019

View reviewed changes

rynowak merged commit b743ba2 into master Mar 20, 2019

rynowak deleted the rynowak/prerendering-error-throwing branch March 20, 2019 20:02

halter73 pushed a commit to dotnet-maestro-bot/AspNetCore that referenced this pull request Mar 28, 2019

Fix link generation of routes with default values (dotnet#8616)

40959a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw unhandled exceptions during prerendering #8616

Throw unhandled exceptions during prerendering #8616

rynowak commented Mar 18, 2019 •

edited

Loading

rynowak Mar 18, 2019

javiercn Mar 20, 2019

rynowak Mar 18, 2019

rynowak Mar 18, 2019

javiercn commented Mar 18, 2019

SteveSandersonMS left a comment

rynowak commented Mar 19, 2019

rynowak Mar 19, 2019

rynowak Mar 19, 2019

SteveSandersonMS left a comment

javiercn Mar 20, 2019

rynowak Mar 20, 2019

Throw unhandled exceptions during prerendering #8616

Throw unhandled exceptions during prerendering #8616

Conversation

rynowak commented Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn commented Mar 18, 2019

SteveSandersonMS left a comment

Choose a reason for hiding this comment

rynowak commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Mar 18, 2019 •

edited

Loading