-
Notifications
You must be signed in to change notification settings - Fork 754
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
Special case Disposing DotNetObjectReferences #2176
Conversation
How does it work in the WASM case? |
It's effectively the same code path. WASM also goes through the dispatcher which is where the code change is in. Am I missing something? |
Nope, you are right, I didn't realize it was an instance method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@@ -66,7 +66,11 @@ module DotNet { | |||
} | |||
} | |||
|
|||
function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, args: any[]): Promise<T> { | |||
function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, ...args: any[]): Promise<T> { | |||
if (assemblyName && dotNetObjectId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just hardening? Or was there a bug here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just hardening. I was confused looking at the function name, because I assumed it only called instance
methods.
@@ -18,7 +16,7 @@ public static partial class DotNetObjectRef | |||
public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class | |||
{ | |||
internal DotNetObjectRef() { } | |||
public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😲
@@ -15,8 +15,7 @@ public static class DotNetObjectRef | |||
/// <returns>An instance of <see cref="DotNetObjectRef{TValue}" />.</returns> | |||
public static DotNetObjectRef<TValue> Create<TValue>(TValue value) where TValue : class | |||
{ | |||
var objectId = DotNetObjectRefManager.Current.TrackObject(value); | |||
return new DotNetObjectRef<TValue>(objectId, value); | |||
return new DotNetObjectRef<TValue>(DotNetObjectRefManager.Current, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to fight with your other change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. But it's easier to review these things piecemeal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are a few small things to clean up here, but overall looks like what I hoped for 👍
This removes a public JSInvokable method required for disposing DotNetObjectReferences
3b8f2d1
to
8e4f4f7
Compare
* Special case Disposing DotNetObjectReferences This removes a public JSInvokable method required for disposing DotNetObjectReferences \n\nCommit migrated from dotnet/extensions@d6bfc28
* Special case Disposing DotNetObjectReferences This removes a public JSInvokable method required for disposing DotNetObjectReferences \n\nCommit migrated from dotnet/extensions@d6bfc28
This removes a public JSInvokable method required for disposing DotNetObjectReferences