Skip to content

Allow JSInvokable identifiers with the same value #1360

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
mrpmorris opened this issue Apr 6, 2019 · 0 comments
Closed

Allow JSInvokable identifiers with the same value #1360

mrpmorris opened this issue Apr 6, 2019 · 0 comments
Assignees

Comments

@mrpmorris
Copy link

See this proposed solution
Pull request

When JavsScript is instructed to call back C# it should not throw an exception about duplicate identifiers when invoking an instance.

public class One
{
  [JSInvokable]
  public void DoSomething() {}
}

public class Two
{
  [JSInvokable]
  public void DoSomething() {}
}

In JavaScript the following code will throw an exception because there is more than one JSInvoke with the identifier "DoSomething"

window.someLibrary = {
  callBackDoSomething: function(callback) {
    callback.invokeMethodAsync("DoSomething");
  }
}

This makes sense when invoking static methods on a static class, but when invoking on a DotNetObjectRef it makes sense to check for duplicate identifiers only on the class of the instance being passed.

This will save from having to write [JSInvokable("MyCompany.MyLibrary.One.DoSomething")] and [JSInvokable("MyCompany.MyLibrary.Two.DoSomething")] to ensure unique callback identifiers across multiple libraries when calling methods on instances (the above string format would still be recommented for static methods though).

In essence, the JSInvokableAttribute applied to a static method should be a globally unique identifier for the method, and on an instance method it should be treated as an alias for the method.

I'd be happy to rewrite the relevant code and submit a pull request if you are in agreement? The implementation I have in mind would also address https://github.com/aspnet/Blazor/issues/1557.

@Eilon Eilon added the area-mvc label Apr 8, 2019
@pranavkm pranavkm self-assigned this Sep 19, 2019
@pranavkm pranavkm added this to the 3.1.0-preview2 milestone Sep 19, 2019
@dougbu dougbu closed this as completed in 659b604 Sep 19, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this issue Feb 12, 2020
…ns#2342)

* Add support for JSInvokable methods on generic types

Prior to this change, DotNetDispatcher cached the MethodInfo on the
generic type definition. Using this would have required MethodInfo.MakeGenericMethod before the method was invoked.
We could separately cache the result of this to avoid the reflection cost per invocation.

Alternatively we could cache static and non-static MethodInfo instances separately which is what this change attempts to do.
The big difference in the outcome is that this requires instance (non-static) JSInvokable methods to be only unique named within
the type hierarchy as opposed to across all static and instance JSInvokable methods in an assembly.

Fixes dotnet/extensions#1360
Fixes #9061
\n\nCommit migrated from dotnet/extensions@659b604
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this issue Feb 13, 2020
…nsdotnet/extensionsdotnet/extensionsdotnet/extensions#2342)

* Add support for JSInvokable methods on generic types

Prior to this change, DotNetDispatcher cached the MethodInfo on the
generic type definition. Using this would have required MethodInfo.MakeGenericMethod before the method was invoked.
We could separately cache the result of this to avoid the reflection cost per invocation.

Alternatively we could cache static and non-static MethodInfo instances separately which is what this change attempts to do.
The big difference in the outcome is that this requires instance (non-static) JSInvokable methods to be only unique named within
the type hierarchy as opposed to across all static and instance JSInvokable methods in an assembly.

Fixes dotnet/extensions#1360
Fixes #9061
\n\nCommit migrated from https://github.com/dotnet/extensions/commit/dotnet/extensions@dotnet/extensions@dotnet/extensions@659b604fb2e595d48a931a7ed831f6c38f035382
\n\nCommit migrated from https://github.com/dotnet/extensions/commit/dotnet/extensions@dotnet/extensions@aaf9159b65e6fd911ffc63514bc3b179d4122d08
\n\nCommit migrated from https://github.com/dotnet/extensions/commit/dotnet/extensions@4f70b0248ef2ba6547cbe93bdea8169c2b826898
\n\nCommit migrated from dotnet/extensions@92c60ff
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this issue Feb 15, 2020
…ns#2342)

* Add support for JSInvokable methods on generic types

Prior to this change, DotNetDispatcher cached the MethodInfo on the
generic type definition. Using this would have required MethodInfo.MakeGenericMethod before the method was invoked.
We could separately cache the result of this to avoid the reflection cost per invocation.

Alternatively we could cache static and non-static MethodInfo instances separately which is what this change attempts to do.
The big difference in the outcome is that this requires instance (non-static) JSInvokable methods to be only unique named within
the type hierarchy as opposed to across all static and instance JSInvokable methods in an assembly.

Fixes dotnet/extensions#1360
Fixes #9061
\n\nCommit migrated from dotnet/extensions@659b604
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants