Skip to content
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

Blazor API Review: JS Interop #12082

Closed
rynowak opened this issue Jul 11, 2019 · 18 comments
Closed

Blazor API Review: JS Interop #12082

rynowak opened this issue Jul 11, 2019 · 18 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed

Comments

@rynowak
Copy link
Member

rynowak commented Jul 11, 2019

Summary

Microsoft.JSInterop is a library that supports bi-directional interop between .NET and JavaScript.

.NET code uses the IJSRuntime or IJSInProcessRuntime interface to dispatch calls to JavaScript.

JavaScript runtimes call into .NET through the DotNetDispatcher class.

.NET API

namespace Microsoft.JSInterop
{
    public static partial class DotNetDispatcher
    {
        public static void BeginInvoke(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
        public static void EndInvoke(string arguments) { }
        public static string Invoke(string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
        [Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
        public static void ReleaseDotNetObject(long dotNetObjectId) { }
    }
    public static partial class DotNetObjectRef
    {
        public static Microsoft.JSInterop.DotNetObjectRef<TValue> Create<TValue>(TValue value) where TValue : class { throw null; }
    }
    public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class
    {
        internal DotNetObjectRef() { }
        public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
        public void Dispose() { }
    }
    public partial interface IJSInProcessRuntime : Microsoft.JSInterop.IJSRuntime
    {
        T Invoke<T>(string identifier, params object[] args);
    }
    public partial interface IJSRuntime
    {
        System.Threading.Tasks.Task<TValue> InvokeAsync<TValue>(string identifier, System.Collections.Generic.IEnumerable<object> args, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
        System.Threading.Tasks.Task<TValue> InvokeAsync<TValue>(string identifier, params object[] args);
    }
    public partial class JSException : System.Exception
    {
        public JSException(string message) { }
        public JSException(string message, System.Exception innerException) { }
    }
    public abstract partial class JSInProcessRuntimeBase : Microsoft.JSInterop.JSRuntimeBase, Microsoft.JSInterop.IJSInProcessRuntime, Microsoft.JSInterop.IJSRuntime
    {
        protected JSInProcessRuntimeBase() { }
        protected abstract string InvokeJS(string identifier, string argsJson);
        public TValue Invoke<TValue>(string identifier, params object[] args) { throw null; }
    }
    [System.AttributeUsageAttribute(System.AttributeTargets.Method, AllowMultiple=true)]
    public partial class JSInvokableAttribute : System.Attribute
    {
        public JSInvokableAttribute() { }
        public JSInvokableAttribute(string identifier) { }
        public string Identifier { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }
    public static partial class JSRuntime
    {
        public static void SetCurrentJSRuntime(Microsoft.JSInterop.IJSRuntime instance) { }
    }
    public abstract partial class JSRuntimeBase : Microsoft.JSInterop.IJSRuntime
    {
        protected JSRuntimeBase() { }
        protected System.TimeSpan? DefaultAsyncTimeout { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        protected abstract void BeginInvokeJS(long taskId, string identifier, string argsJson);
        protected internal abstract void EndInvokeDotNet(string callId, bool success, object resultOrError, string assemblyName, string methodIdentifier, long dotNetObjectId);
        public System.Threading.Tasks.Task<T> InvokeAsync<T>(string identifier, System.Collections.Generic.IEnumerable<object> args, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
        public System.Threading.Tasks.Task<T> InvokeAsync<T>(string identifier, params object[] args) { throw null; }
    }
}

TS API

// This is a single-file self-contained module to avoid the need for a Webpack build

module DotNet {
  (window as any).DotNet = DotNet; // Ensure reachable from anywhere

  export type JsonReviver = ((key: any, value: any) => any);

   * Sets the specified .NET call dispatcher as the current instance so that it will be used
   * for future invocations.
   *
   * @param dispatcher An object that can dispatch calls from JavaScript to a .NET runtime.
   */
  export function attachDispatcher(dispatcher: DotNetCallDispatcher) { }

  /**
   * Adds a JSON reviver callback that will be used when parsing arguments received from .NET.
   * @param reviver The reviver to add.
   */
  export function attachReviver(reviver: JsonReviver) { }

  /**
   * Invokes the specified .NET public method synchronously. Not all hosting scenarios support
   * synchronous invocation, so if possible use invokeMethodAsync instead.
   *
   * @param assemblyName The short name (without key/version or .dll extension) of the .NET assembly containing the method.
   * @param methodIdentifier The identifier of the method to invoke. The method must have a [JSInvokable] attribute specifying this identifier.
   * @param args Arguments to pass to the method, each of which must be JSON-serializable.
   * @returns The result of the operation.
   */
  export function invokeMethod<T>(assemblyName: string, methodIdentifier: string, ...args: any[]): T { }

  /**
   * Invokes the specified .NET public method asynchronously.
   *
   * @param assemblyName The short name (without key/version or .dll extension) of the .NET assembly containing the method.
   * @param methodIdentifier The identifier of the method to invoke. The method must have a [JSInvokable] attribute specifying this identifier.
   * @param args Arguments to pass to the method, each of which must be JSON-serializable.
   * @returns A promise representing the result of the operation.
   */
  export function invokeMethodAsync<T>(assemblyName: string, methodIdentifier: string, ...args: any[]): Promise<T> { }

  /**
   * Represents the ability to dispatch calls from JavaScript to a .NET runtime.
   */
  export interface DotNetCallDispatcher {
    /**
     * Optional. If implemented, invoked by the runtime to perform a synchronous call to a .NET method.
     * 
     * @param assemblyName The short name (without key/version or .dll extension) of the .NET assembly holding the method to invoke. The value may be null when invoking instance methods.
     * @param methodIdentifier The identifier of the method to invoke. The method must have a [JSInvokable] attribute specifying this identifier.
     * @param dotNetObjectId If given, the call will be to an instance method on the specified DotNetObject. Pass null or undefined to call static methods.
     * @param argsJson JSON representation of arguments to pass to the method.
     * @returns JSON representation of the result of the invocation.
     */
    invokeDotNetFromJS?(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, argsJson: string): string | null;

    /**
     * Invoked by the runtime to begin an asynchronous call to a .NET method.
     *
     * @param callId A value identifying the asynchronous operation. This value should be passed back in a later call from .NET to JS.
     * @param assemblyName The short name (without key/version or .dll extension) of the .NET assembly holding the method to invoke. The value may be null when invoking instance methods.
     * @param methodIdentifier The identifier of the method to invoke. The method must have a [JSInvokable] attribute specifying this identifier.
     * @param dotNetObjectId If given, the call will be to an instance method on the specified DotNetObject. Pass null to call static methods.
     * @param argsJson JSON representation of arguments to pass to the method.
     */
    beginInvokeDotNetFromJS(callId: number, assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, argsJson: string): void;
  }

  /**
   * Receives incoming calls from .NET and dispatches them to JavaScript.
   */
  export const jsCallDispatcher = {
    /**
     * Finds the JavaScript function matching the specified identifier.
     *
     * @param identifier Identifies the globally-reachable function to be returned.
     * @returns A Function instance.
     */
    findJSFunction, // Note that this is used by the JS interop code inside Mono WebAssembly itself

    /**
     * Invokes the specified synchronous JavaScript function.
     *
     * @param identifier Identifies the globally-reachable function to invoke.
     * @param argsJson JSON representation of arguments to be passed to the function.
     * @returns JSON representation of the invocation result.
     */
    invokeJSFromDotNet: (identifier: string, argsJson: string) => { },

    /**
     * Invokes the specified synchronous or asynchronous JavaScript function.
     *
     * @param asyncHandle A value identifying the asynchronous operation. This value will be passed back in a later call to endInvokeJSFromDotNet.
     * @param identifier Identifies the globally-reachable function to invoke.
     * @param argsJson JSON representation of arguments to be passed to the function.
     */
    beginInvokeJSFromDotNet: (asyncHandle: number, identifier: string, argsJson: string): void => {  },

    /**
     * Receives notification that an async call from JS to .NET has completed.
     * @param asyncCallId The identifier supplied in an earlier call to beginInvokeDotNetFromJS.
     * @param success A flag to indicate whether the operation completed successfully.
     * @param resultOrExceptionMessage Either the operation result or an error message.
     */
    endInvokeDotNetFromJS: (asyncCallId: string, success: boolean, resultOrExceptionMessage: any): void => { }
}
@rynowak rynowak added API Review area-blazor Includes: Blazor, Razor Components labels Jul 11, 2019
@rynowak
Copy link
Member Author

rynowak commented Jul 12, 2019

Some of my thoughts about this.... I'm going to stick to the .NET side because I'm not entirely certain what to optimize for in the TS apis.

DotNetDispatcher

I'd like to choose a name or namespace for dedicated interop types. Previously .NET has given the guidance to use specific names. This kind of thing is valuable because it helps organization and works with tooling like analyzers (can teach analyzers to ignore that namespace).

We're also planning to build an analyzer that will warn on usage of unsupported APIs (for ASP.NET Core as a whole). This is something we could leverage here as well - (don't call interop APIs from .NET).

I'm thinking about that because I look at some of these signatures, and I think they would break if we added more features. For instance, what about if we added support for generics? That would require adding another new API since we'd have no way to pass a type in there. Adding a parameter object would help.

Rather than trying to polish what's here to a mirror sheen, I think it's a better idea to declare this area as "implementation detail only".

JSAsyncCallResult should probably move to this namespace.

JSInvokableAttribute

JSInvokableAttribute - this should be sealed and inherited = true. I'm not sure why we decided on AllowMultiple? Is this to support multiple names per method? Unless we have a test for that I would be really surprised if it works.

DotNetObjectRef

We can write a custom converter to get rid of __dotNetObject.

We need to lock on the name of this. I'm not a big fan of seeing DotNet in names, it seems really not nice to read 😢 I'm also not a fan of Ref since we don't generally abbreviate in names.

Some suggestions:

  • ObjectReference
  • JSReference

I don't want to drill into this too hard, if there isn't another suggestion that we really like, then it should stay as is.

IJSRuntime and related types

We should consider using ValueTask<>. This would save an allocation in in-process scenarios.

I'm not totally certain why we have separate interfaces and abstract classes. My main concern is that we can't easily add members to the interfaces, so I'm not sure what exactly they buy us vs just the classes.

BeginInvokeJS could have its name improved, like making it consistent with InvokeAsync (remove the JS). Some names could be made less idiosyncratic in general in the implementations.

I'm not sure that my concerns about the security of InvokeAsync and the semantics of passing the default cancellation token were ever addressed.

OnDotNetInvocationException seems like a really fragile contract. It's not clear why the method and assembly are arguments here vs a parameter object. Imagine we added support for generics, this signature is broken now.

@rynowak rynowak self-assigned this Jul 12, 2019
@rynowak rynowak added this to the 3.0.0-preview8 milestone Jul 12, 2019
@danroth27 danroth27 mentioned this issue Jul 15, 2019
20 tasks
@pranavkm
Copy link
Contributor

Throwing in JSMarshalByRefObject as a suggestion for the name

I wanted to consider removing the DotNetObjectRef.Create API and instead have an API on IJSRuntime \ JSRuntimeBase that exposed this. Right now, the Create API statically gets at the JSRuntimeBase instance. Having it be on the runtime makes it a bit more obvious that it's the one managing the lifetime of the reference and avoids creating spooky issues like #11159.

Proposal

JSMarshalByRefObject<T> IJSRuntime.CreateMarshalByRefObject(T value) where T : object

In addition to this, as part of the Dispatcher's argument binding, we should also consider binding IJSRuntime instances with the current \ correct value. This would prevent users from having to figure this out themselves in case they need to call back in to the IJSRuntime as also required if we decide to go with my first suggestion. You will need an instance of IJSRuntime to do this: https://github.com/aspnet/AspNetCore/blob/87a92e52c8b4bb7cb75ff78d53d641b1d34f8775/src/Components/test/testassets/BasicTestApp/ServerReliability/JSInterop.cs#L8-L12

@pranavkm
Copy link
Contributor

pranavkm commented Jul 30, 2019

Another follow: calls to DotNetDispatcher typically involve looking up the instance of JSRuntimeBase using the JSRuntime.Current AsyncLocal instance. Could we consider a model where the runtime is passed in rather than relying on ambient state? Makes the API much more explicit and easier to reason about:

public static partial class DotNetDispatcher
{
    public static void BeginInvoke(JSRuntimeBase jsRuntimeBase, string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
    
    public static void EndInvoke(JSRuntimeBase jsRuntimeBase, string arguments) { }
    
    public static string Invoke(JSRuntimeBase jsRuntimeBase, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
    
    
    [Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
    public static void ReleaseDotNetObject(JSRuntimeBase jsRuntimeBase, long dotNetObjectId) { }
}

The only interesting behavior to add would for JSInvokable methods. We would "bind" the JSRuntime instance to a parameter if it's assignable from JSRuntimeBase, kinda how https://github.com/aspnet/Extensions/blob/master/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs#L200-L208 currently works.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 8, 2019

  • DotNetDispatcher

    • figure out what namespace -> Microsoft.JsInterop.Infrastructure
    • pattern for JSInvokable. Consider putting them in well-known type per assembly.
    • ReleaseDotNetObject - what to do - Make this not JSInvokable (Fixed as part of dotnet/extensions@d6bfc28)
    • Naming: BeginInvokeDotNet
    • Naming: EndInvokeJS
  • DotNetObjectRef

    • Naming: DotNetObjectReference
    • File an issue to consider removing JSRuntime.Current access
  • DotNetObjectRef

    • Naming: DotNetObjectReference
    • Figure out what happens if we need to guard against the reference instance being serialized by different JSRuntime than the one that was created. We should disallow this or make it work reliably (prefer the former if this is too much work).
  • IJSRuntime

    • Add ValueTask InvokeVoidAsync(string identifier, IEnumerable<object> objects, CancellationToken token);
    • Change Task<TValue> -> ValueTask<TValue>
    • Make System.Threading.Tasks.Task InvokeAsync(string identifier, params object[] args); an extension method
    public interface IJSRuntime
    {
        ValueTask<TValue> InvokeAsync<TValue>(CancellationToken token, string identifier, IEnumerable<object> object);
        ValueTask<TValue> InvokeAsync<TValue>(string identifier, IEnumerable<object> object);
    }
    
    public static class JSRuntimeExtensions
    {
        ValueTask InvokeVoidAsync(CancellationToken token, string identifier, params object[] args);
        ValueTask InvokeVoidAsync(string identifier, params object[] args);
        ValueTask<TValue> InvokeAsync(string identifier, params object[] args);
        // Document that a user can explicitly pass Timeout.Infinite if you want infinite timeout
        ValueTask<TValue> InvokeAsync(TimeSpan timeSpan, string identifier, params object[] args);
    }

    https://github.com/aspnet/Extensions/blob/master/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs#L46
    Change this to say if (cts == default) { UseDefaultTimeout; }

  • JSException -> include a value that says if it 's a parse error

  • Rename JSRuntimeBase -> JSRuntime

    • protected abstract void BeginInvokeJS(long taskId, string identifier, string argsJson);
      Change taskId to ulong

    • Change protected internal abstract void EndInvokeDotNet(string callId, bool success, object resultOrError, string assemblyName, string methodIdentifier, long dotNetObjectId); to

    protected internal abstract void EndInvokeDotNet(string callId, bool success, object resultOrError, DotNetErrorInfo errorInfo); and move fields in to it.

    readonly struct DotNetErrorInfo
    {
    public string AssemblyName { get; }
    public string MethodIdentifier { get; }
    public string DotNetObjectId { get; }
    FailureKind FailureKind
    }

    enum FailureKind
    {
    InvocationFailure,
    ParseFailure,
    InvalidDotNetObjectReference
    }

  • Rename JSInProcessRuntimeBase -> JSInProcessRuntime

    public static class JSInProcessRuntimeExtension
    {
        void InvokeVoid(this IJSRuntimeBase base, string identifier, params object[] args)
    }
  • JSInvokableAttribute -> Make it sealed

  • JSRuntime:

    • SetCurrentJSRuntime - move to DotNetDispatcher
    • Remove type
  • Cleanup https://github.com/aspnet/AspNetCore/blob/771989b71dbfcdfed4093a5087948fbe87445bd6/src/Components/Server/src/Circuits/RemoteJSRuntime.cs#L32

@rynowak
Copy link
Member Author

rynowak commented Aug 8, 2019

I caught up with @terrajobst to talk about the methods on IJSRuntime and our current POR. His thoughts were basically:

  • Having two methods on the interface are fine, it's probably the best way to get what we want, and is appropriate because there are far more callers than implementors
  • Make the CancellationToken the second parameter because a string is far more likely to appear in the list of parameters passed to JS than a cancellation token.
  • Our lives will be simpler if we don't need the IEnumerable<object> family of methods, and are OK just using params object[].

As an example of this last one, consider what happens when you need to pass a List<Customer> as a single parameter to JS. That will convert to IEnumerable<object>. So any time someone is calling a JS method with a single parameter, and that parameter is a collection, they need to know about this quirk.

If we just do object[] then this case only occurs with arrays, which are less common. There's no great mitigation for this all-up.

@rynowak
Copy link
Member Author

rynowak commented Aug 8, 2019

We also talked about using Span, but I'm not sure that applies because this is an async method. I think we're probably smart enough to implement this combo, but I think about people mocking the API and stuff.

@terrajobst
Copy link
Member

terrajobst commented Aug 8, 2019

but I think about people mocking the API and stuff.

That's a fair, but if that's a concern we could probably provide an implementation for mocking that takes a delegate. I'd say it depends on how valuable you see the reduction of allocations for JS interop.

@egil
Copy link
Contributor

egil commented Aug 14, 2019

@rynowak, I do not know if its too late to provide input here, but ill try anyway :)

When building component libraries, we do not know if the user will be using them in a blazor client or blazor server scenario, and if the client will have prerenderinger enabled.

That means we must guard against a disconnected state, inject IComponentContext into our components and services, and check with IComponentContext.IsConnected before calling our JavaScript functions.

For components, it looks as if we can skip the JS call if IsConnected = false, since the components will get re-rendered again when the connection is established and then we get a second chance to call. E.g. the pattern looks a bit like this:

[Inject] private IComponentContext? Context { get; set; }

protected override Task OnAfterRenderAsync()
{
    if (!Context.IsConnected)
        return Task.CompletedTask;
    else
        return JsRuntime!.InvokeAsync<object>("jsFunc", ...);
}

For services (scoped/singleton) it is a bit more tricky as far as I can tell, since the service only gets instantiated once (during first render). Thus, if we have a DoSomethingAsync() method that uses JS and it can be called by a component, we must spin in the service, waiting for the connection to be established. E.g.:

public class MyService : IDisposable
{
    private readonly IJSRuntime _jsRuntime;
    private readonly IComponentContext _componentContext;
    private CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();

    public MyService(IJSRuntime jsRuntime, IComponentContext componentContext)
    {
        _jsRuntime = jsRuntime;
        _componentContext = componentContext;
    }

    public async Task DoSomethingAsync()
    {
        var isConnected = await _componentContext.IsConnectedAsync(_cancellationTokenSource.Token);
        if (isConnected)
        {
            await _jsRuntime.InvokeAsync<object>("jsFunc", Array.Empty<object>(), _cancellationTokenSource.Token);
        }
    }

    public void Dispose()
    {
        _cancellationTokenSource.Cancel();
        _cancellationTokenSource.Dispose();
        _cancellationTokenSource = null;
    }
}

The above code is possible with this extension method:

public static class ComponentContextExtensions
{
    public static async Task<bool> IsConnectedAsync(this IComponentContext componentContext, CancellationToken token = default)
    {
        while (!componentContext.IsConnected && !token.IsCancellationRequested)
        {
            await Task.Delay(50, token);
        }
        return componentContext.IsConnected;
    }
}

The above pattern for services is probably not always needed. I have one case where I think it is though: I have a service that subscribes to particular DOM events (Page Visibility API) when the first component subscribes to it. That way, one or more components needing to know about that DOM event only results in one subscription on the DOM. However, if IsConnected is false, it spins as shown in the example above, and subscribes to the DOM event as soon as it is possible. The component thus doesn't have to deal with connected or disconnected states, which is nice, since they are just subscribing to the service using regular .net events.

To sum up, I think the patterns I've shown above are OK, but it would be nice if there was an easier way, perhaps with a little more help from the JSRuntime/ComponentContext APIs, to deal with the connected/disconnected scenarios.

@rynowak
Copy link
Member Author

rynowak commented Aug 15, 2019

Thanks for reaching out. The following is out of date:

When building component libraries, we do not know if the user will be using them in a blazor client or blazor server scenario, and if the client will have prerenderinger enabled. That means we must guard against a disconnected state, inject IComponentContext into our components and services, and check with IComponentContext.IsConnected before calling our JavaScript function

OnAfterRender is no longer called in prerendering. We're actually removing IComponentContext because we've solved this problem by just making OnAfterRender fit for purpose.

Thus, if we have a DoSomethingAsync() method that uses JS and it can be called by a component, we must spin in the service, waiting for the connection to be established

I'd suggest passing this problem to your caller. Code that calls into JS interop is going to have to be prepared for exceptions coming from the JS side. It's not expected that JS interop is bulletproof - because the JS side is untrusted code, and the connection is not guaranteed to have 100% uptime.

@egil
Copy link
Contributor

egil commented Aug 15, 2019

OnAfterRender is no longer called in prerendering. We're actually removing IComponentContext because we've solved this problem by just making OnAfterRender fit for purpose.

Is that in P8? I just did a quick dotnet new blazorserver -o test, added a <Test /> component to the Shared folder with the following content:

@code {
  protected override void OnAfterRender() {
    Console.WriteLine("Test.OnAfterRender");
  }
}
<h5>Hello from Test</h5>

When the <Test /> component is included in Index.razor and I do a dotnet run and browse to it, I see two calls to Test.OnAfterRender. If I remove prerendering from _Hosts.cshtml, I only see one call.

I'd suggest passing this problem to your caller. Code that calls into JS interop is going to have to be prepared for exceptions coming from the JS side. It's not expected that JS interop is bulletproof - because the JS side is untrusted code, and the connection is not guaranteed to have 100% uptime.

I am not sure I agree with this. It seems like my service would be violating a bunch of SOLID principles, if all the users of my service have to deal with possible JS interop code. It seems better to me to have the service take responsibility and shield the users as much as possible, and in that sense, giving the service the possibility to detect when the JS connection is down is not a bad idea. Especially in scenarios as the one I tried to describe in the end of my previous post.

Thus, I hope you will continue to provide a way for services to detect if JS interop is present and available, and at least provide some guidance on how and when to the connection can be dropped, when a service should give up on it entirely, and when it might come back.

@julienGrd
Copy link

Hi guys, i still have the error "JSRuntime must be set up correctly and must be an instance of JSRuntimeBase to use DotNetObjectRef.'" when i try to inject component in js. I don't know if it still "normal" with preview 8 ?

protected override async Task OnAfterRenderAsync()
        {
            if (ComponentContext.IsConnected)
            {
                try
                {
                    await this.InvokeJsAsync<object>("injectModalManager", DotNetObjectRef.Create(this));
                }
                catch (Exception ex)
                {
                    //https://github.com/aspnet/AspNetCore/issues/11159
                }
        }
}

thanks !

@egil
Copy link
Contributor

egil commented Aug 15, 2019

Hi guys, i still have the error "JSRuntime must be set up correctly and must be an instance of JSRuntimeBase to use DotNetObjectRef.'" when i try to inject component in js. I don't know if it still "normal" with preview 8 ?

Ryan writes in the related issue that it will be solved when this is issue is resolved, so the answer is yes.

@rynowak
Copy link
Member Author

rynowak commented Aug 16, 2019

Is that in P8?

This is preview 9. I just checked with an example similar to yours to make sure we got it right.

Unfortunately the team is working a few weeks ahead of the public release right now. Preview 8 just shipped this week, but as of today we're done with preview 9 changes 😆 I realize that's confusing to follow along with.

I am not sure I agree with this. It seems like my service would be violating a bunch of SOLID principles, if all the users of my service have to deal with possible JS interop code. It seems better to me to have the service take responsibility and shield the users as much as possible, and in that sense, giving the service the possibility to detect when the JS connection is down is not a bad idea. Especially in scenarios as the one I tried to describe in the end of my previous post.

I realize that may not be convenient but it's unavoidable. Any time you have a pattern like if (connected) { DoStuff(); } you have the possibility of a race condition blowing things up on. Hopefully these kinds of problems are rare in practice.

You also have to keep in mind that JS interop runs arbitrary code on the client. Your client might be running JS code that you didn't write. A hostile client might stub out some JS call to never return, or return bad results. Basically anywhere you're using JS interop, you're going to want to harden that, including tasks that get cancelled or connection failures.

Thus, I hope you will continue to provide a way for services to detect if JS interop is present and available, and at least provide some guidance on how and when to the connection can be dropped, when a service should give up on it entirely, and when it might come back.

Can you log a new issue and include some examples (including how you want to handle disconnected cases in your APIs)? I'm happy to continue a discussion on this topic, and do more work in the future to enable you. I suspect that IComponentContext ultimately would have been too primitive for your needs.

@rynowak
Copy link
Member Author

rynowak commented Aug 16, 2019

@julienGrd - please log a new issue and don't piggyback on the this API review discussion 😆

@pranavkm pranavkm removed the Working label Aug 17, 2019
@pranavkm pranavkm added the Done This issue has been fixed label Aug 17, 2019
@pranavkm
Copy link
Contributor

Done.

@egil
Copy link
Contributor

egil commented Aug 17, 2019

Can you log a new issue and include some examples (including how you want to handle disconnected cases in your APIs)? I'm happy to continue a discussion on this topic, and do more work in the future to enable you. I suspect that IComponentContext ultimately would have been too primitive for your needs.

Thanks. I will see what is possible and what is not when P9/RC lands. Then, I might take you up on that offer. Thanks again.

@rynowak
Copy link
Member Author

rynowak commented Aug 18, 2019

Thanks. I will see what is possible and what is not when P9/RC lands. Then, I might take you up on that offer.

Cool, what you should expect is that these scenarios have simpler guidance:

  • OnAfterRender is not called in prerendering
  • There's a firstRender parameter to make it more idoimatic to initialize JS Interop for a component
  • IComponentContext is removed

The main reason we added IComponentContext was to make it possible for a component to know if it was in prerendering or not. The fact that it exposed whether or not you were connected was a detail that came up during implementation as the most sensible way to describe the API. I think it's problematic for the kinds of use cases you described, because it doesn't expose eventing or asynchrony - there's no way to be notified when the state changes.

The best course of action for us was to remove it for the 3.0 release, because we didn't really think it offered any value.

@egil
Copy link
Contributor

egil commented Aug 18, 2019

OK @rynowak, let me share my current thoughts, maybe you can fill in a few gaps :)

My scenario is primarily a component library/service library setting. Ideally, since Razor components leverages dependency injection, the users of my library doesn't need to know or should know whether or not the services or components in my library uses JSInterop. If they do, they would, like you say, have to be aware and handle of the various pitfalls with JSInterop.

If I later remove a components or services dependency on JSInterop (you guys might release new APIs later that means I don't need it), then all my users who have been good Razor citizens and added proper error handling for JSInterop, they would likely need to clean up their code again.

With the above in mind, I will as a library author always prefer to handle the expected JSInterop errors, and wrap the unexpected JSInterop errors in a custom exception type, that generally signal to users of my library that something unexpected happened. That way, the users of my libraries can have a more simplified error handling, or none at all, and at least one that is less likely to change.

Known or expected errors

This is where you can probably help -- what are the all the things that can go wrong in JSInterop world, and how to recognize each one?

  1. Prerendering - JSInterop is not available. Most likely cause of action is to simple continue and not do anything.
  2. JSInterop call doesn't go through or doesn't complete. Cause of action depends on scenario, but a circuit breaker approach could be a possibility in some cases. At least retry N times with X delay.
  3. Called JS function doesn't exist (variant of 2. option). Cause of action could probably try again after a short delay, since browser might not have completed downloading/parsing the source javascript file, then after a little while throw an exception
  4. Mangled response. Cause of action is very dependent on context. If we worry about security implications due to the scenario, then maybe throw an cannot continue exception and have the user of the service or component decide what to do. If it is more benign, like my case with the Page Visibility API, then maybe simple ignore the response and wait for a next one.

There might be other considerations for client side blazor, and I cannot claim to have any experience with SingalR directly, so there might also be some protocol-level errors that can be handled gracefully, depending on scenario/context.

Thoughts?

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

5 participants