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

Reconsider static JSRuntime.Current #6828

Closed
SteveSandersonMS opened this Issue Jan 18, 2019 · 5 comments

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 18, 2019

JSRuntime.Current is misleading. People commonly write code like this:

public override void OnInit()
{
    StocksService.OnStockTickerUpdated += stockUpdate =>
    {
        JSRuntime.Current.InvokeAsync<object>(
            "handleTickerChanged", stockUpdate.symbol, stockUpdate.price);
    };
}

This is fine for client-side code, but when executing on the server, which user's browser is it sending the handleTickerChanged call to? The developer probably thinks it's the user whose browser is rendering the component that contains this call, but that's not true. In fact, it depends on the async context in which the OnStockTickerUpdated event fires - maybe it's some other user who caused the event, or maybe it's nobody at all. When testing locally there's a good chance it seems to work, but the problem of sending the call to the wrong user only surfaces in production when there is more than one user.

The underlying problem is having a static JSRuntime.Current. If, instead, you could only acquire the IJSRuntime from DI, then each component would get and store the right instance for its user, and the call would always be sent to the right user. It's already possible to get the IJSRuntime from DI, but people don't realise that's what they should do so they continue to use JSRuntime.Current.

Proposal: Just remove the static JSRuntime.Current, so you can only get it from DI.

Drawback: in the pure client-side Blazor scenario, this complicates things for library code that doesn't necessarily have access to DI. It forces the developer to obtain the IJSRuntime from DI and pass it into the library somehow. However I still think this is the right thing to do, because if libraries are ever going to work in both client and server code, they have to let the developer pass the IJSRuntime into them.

@SteveSandersonMS

This comment has been minimized.

Copy link
Member Author

SteveSandersonMS commented Jan 18, 2019

@SteveSandersonMS SteveSandersonMS added this to To do in Razor Components via automation Jan 18, 2019

@SteveSandersonMS SteveSandersonMS moved this from To do to Design in Razor Components Jan 18, 2019

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Jan 18, 2019

I support this 😬

@Eilon Eilon added the area-mvc label Jan 18, 2019

@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview3 milestone Feb 6, 2019

pranavkm added a commit that referenced this issue Feb 15, 2019

pranavkm added a commit that referenced this issue Feb 15, 2019

pranavkm added a commit that referenced this issue Feb 15, 2019

pranavkm added a commit that referenced this issue Feb 15, 2019

@pranavkm pranavkm added 2 - Working and removed 1 - Ready labels Feb 15, 2019

pranavkm added a commit to aspnet/Extensions that referenced this issue Feb 15, 2019

@rynowak rynowak moved this from Design to In progress in Razor Components Feb 15, 2019

pranavkm added a commit to aspnet/Extensions that referenced this issue Feb 15, 2019

pranavkm added a commit that referenced this issue Feb 15, 2019

Remove uses of JSRuntime.Current (#7599)
* Remove uses of JSRuntime.Current

Prerequisite for #6828
@wicked-sick

This comment has been minimized.

Copy link

wicked-sick commented Feb 18, 2019

@SteveSandersonMS What is the recommended way for injecting IJSRuntime if the component does not have a .cshtml file, e.g. when it relies on dynamic content by overriding BuildRenderTree and thus cannot use the @inject helper?

@javiercn

This comment has been minimized.

Copy link
Member

javiercn commented Feb 18, 2019

@wicked-sick You can just put [Inject] IJSRuntime JSRuntime { get; set; } and that should do it.

Razor Components automation moved this from In progress to Done Feb 19, 2019

pranavkm added a commit to aspnet/Extensions that referenced this issue Feb 19, 2019

@pranavkm pranavkm added 3 - Done and removed 2 - Working labels Feb 19, 2019

@pranavkm

This comment has been minimized.

Copy link
Member

pranavkm commented Feb 19, 2019

As noted here, for preview3, JSRuntime.Current is internal. We could take another stab at it as part of aspnet/AspNetCore-Internal#1678 (I've noted it as a thing to do)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.