Skip to content

[Blazor server-side] Improved logging for JS interop #12182

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

Closed
wants to merge 16 commits into from

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jul 15, 2019

Original logging PR + structural changes to support routing events and .NET interop through specific paths separate from JS interop.

Associated PR with JSInterop changes to support this PR dotnet/extensions#2043

  • Dispatches events through a method hub to avoid going through JS interop for browser events on server-side blazor.
  • Dispatches JS async completions through a method hub to avoid going through JS interop for JS async method completions.
  • Replaces OnDotNetInvocationException with EndInvokeDotNet to provide better diagnostics (explicitly differentiate successful calls from invalid ones)
    • Additionally, tentatively allows us to dispatch JS .NET async calls through a method hub instead of through JS interop if we choose to do so in the future.
  • Add some tests for the logging.

@javiercn javiercn changed the title Javiercn/improved logging with hub methods [Discussion][Blazor server-side] Reroute events and .NET interop through hub methods. Jul 15, 2019
eventArgsJson = element.GetString();
}
}
var _ = EnsureCircuitHost().DispatchEvent(eventDescriptor, eventArgsJson);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a way to differentiate between what it is essentially well-formed bad input (like an event for a non-existing handler) and exceptions in user code (Exceptions thrown inside developer's event handling code).

@SteveSandersonMS
Copy link
Member

@rynowak Javier and I discussed this today and quite a lot of the shape of it is going to change. The existing JS interop contract makes it hard to understand whether things succeeded or failed, leading to this PR duplicating various bits of the parsing logic, and some odd special-case logic on the TS side. The plan now is to change the JS interop contract to expose the specific data needed for a platform implementation to log it sensibly.

So you might want to hold off on reviewing it for now.

@javiercn
Copy link
Member Author

I spoke with Steve and we are doing some adjustments.

  • We are adding a concept to abstract dispatching events so that the server can override it directly.
    • The server side implementation will send the descriptor and the args as two separate arguments and we will simply deserialize the descriptor on the circuit host before passing it to the renderer.
  • EnvInvoke will also take a string as an argument and will perform the deserialization in the normal way inside JS interop.
    • There will be an endInvokeJS hook available in JS interop for handling the details of completing calls.
    • The server will override this hook to send a bolean flag with success or failure and the arguments to the call.

Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really interested to see thoughts from @SteveSandersonMS - I see this kind of thing as a pure win - making these features first-class rather than tunneled though the same layer as app code means that we can have them break the rules if necessary. I know this is something we've debated in the past 😆

await Renderer.Dispatcher.InvokeAsync(() =>
{
SetCurrentCircuitHost(this);
return RendererRegistryEventDispatcher.DispatchEvent(eventDescriptor, eventArgsJson);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same deal. There's no more need for a magic static in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can dispatch the event directly to the renderer, we would need to have a method to interpret the event args. https://github.com/aspnet/AspNetCore/blob/a784f4575b738ec5b7c6f5cfff2b1fee10de5118/src/Components/Web/src/RendererRegistryEventDispatcher.cs#L24-L25

@javiercn javiercn force-pushed the javiercn/improved-logging-with-hub-methods branch from 14712df to f80094b Compare July 15, 2019 22:05
@javiercn javiercn changed the base branch from javiercn/improved-logging to master July 15, 2019 22:06
@javiercn
Copy link
Member Author

🆙 📅

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! This looks really good!

@javiercn
Copy link
Member Author

The commits are included in this PR are included in #12250 which will be merged with the rest of the dependency updates, so closing this PR in favor of that.

@javiercn javiercn closed this Jul 18, 2019
@javiercn javiercn deleted the javiercn/improved-logging-with-hub-methods branch July 18, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants