Skip to content

Conversation

@MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Aug 18, 2020

Summary

  • Modified JS interop to use the new mono_wasm_invoke_js_blazor function.
  • Added support for indicating special JS interop return values (e.g., a JSObjectReference), along with the ability to specify which object instance is handling the invocation.
  • Added JSObjectReference, which supports invoking functions from JS objects.
  • Added the ability to pass a JSObjectReference instance as a parameter to a JS interop call.
  • Added support for synchronous JS interop calls via JSInProcessObjectReference.
  • Added DotNet.createJSObjectReference to allow passing JS objects to .NET. and DotNet.disposeJSObjectReference to allow disposal from JS when it's most convenient.
  • Added some functional/E2E tests.

TODO

  • Add support for unmarshalled JS interop calls via JSUnmarshalledObjectReference (dependent on InputFile Component #24640).
    • Update: This will likely get done in a follow-up PR, as it's not a critical piece of functionality.

Addresses #24648

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks like a great start!

@MackinnonBuck MackinnonBuck requested a review from pranavkm August 20, 2020 18:58
@MackinnonBuck MackinnonBuck marked this pull request as ready for review August 20, 2020 20:45
@MackinnonBuck MackinnonBuck requested review from a team and SteveSandersonMS as code owners August 20, 2020 20:45
@MackinnonBuck MackinnonBuck changed the base branch from master to release/5.0 August 20, 2020 22:23
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

This looks fantastic so far!

I don't have a lot of meaningful feedback because everything seems to be very well thought/written!

I would like to see a few more unit tests and so on, but other than that, it looks great!

We might also want the equivalent of DotnetObjectRef.Create on the JS side so that JS objects can be turned into JSObjectReferences for use when invoking .NET.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Aug 21, 2020

This looks pretty super to me. I haven't pored over every line of it yet, or tried to think of any ways it could be misused, but it's looking very strong so far.

[Unmarshalled calls] Update: This will likely get done in a follow-up PR, as it's not a critical piece of functionality.

It sounds fine to me that we could add unmarshalled JSObject support in RC2. It's the sort of change that would have a really clear boundary and be quite easy to test. And there would be limited concerns around security since by definition it can only exist on the WebAssembly runtime.

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 24, 2020
@pranavkm pranavkm added this to the 5.0.0-rc1 milestone Aug 24, 2020
@pranavkm pranavkm added area-blazor Includes: Blazor, Razor Components and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 24, 2020
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Learned a lot while reading through your PR. Left a few questions inline.

@MackinnonBuck MackinnonBuck merged commit 8a2f29b into release/5.0 Aug 25, 2020
@MackinnonBuck MackinnonBuck deleted the t-mabuc/js-object-reference branch August 25, 2020 04:02
@mrlife
Copy link
Contributor

mrlife commented Sep 5, 2020

Will JSObjectReference be available in Server-Side Blazor or is it a WASM-only feature?

@SteveSandersonMS
Copy link
Member

@mrlife It works on both.

@RemiBou
Copy link
Contributor

RemiBou commented Sep 8, 2020

Congrats for this PR, I did this here but in way less efficient way I think. Can't wait to remove all my code!

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.

9 participants