Skip to content

Conversation

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Sep 4, 2020

Note This PR is based on javiercn/css-isolation-follow-ups, not because I actually want to merge into that branch, but because it builds on functionality added in #25565 and can't be merged until that one is.

Description

This PR improves the Razor Class Library template to use 5.0 features:

  • CSS isolation, so that (1) styles are scoped, and (2) consumers of the library no longer have to add any <link> tag for the library's styles - it's auto-bundled.
  • JS isolation, so that (1) it doesn't pollute the global namespace, and (2) consumers of the library no longer have to add any <script> tag for the library's JS - it's auto-loaded on demand.

Customer Impact

Makes clear how the 5.0 features are meant to be used in RCLs, and avoids the weirdness of shipping an RCL template that inexplicably fails to use the features that are obviously designed for this scenario.

Regression

No

Risk

As long as we actually do some verification on this, very low chance of causing any bugs.

Design note

It does make the ExampleJsInterop.cs code quite a bit longer and involves more concepts.

Previously it was just a few-lines-long static function you could invoke and pass in an IJSRuntime instance, and it relied on the associated .js file already being loaded in the global namespace. The new version is set up to be used as a DI service (not a static) and contains code to lazy-load the associated .js file.

I've made it an IAsyncDisposable because you could think of it as a good practice to remind people that IJSObjectReference instances are IAsyncDisposable. In practice, as long as ExampleJsInterop is consumed as a scoped or singleton instance, there would be no need ever to dispose the lazy-loaded JS module so it's kind of redundant, but I think we should do it like I have here in order to be more risk averse and not set a bad example that disposability can be ignored.

On balance I think the new version of ExampleJsInterop.cs is more useful than the previous one and guides people to be more successful, so it's worth the extra 20 lines of code there.

Design note 2

Most of the lines of code in ExampleJsInterop.cs could be factored out into an abstract base class, e.g., JSModuleBase. This could contain methods like InvokeAsync(...) etc. that pass through the call to (await ModuleTask).InvokeAsync(...) so you wouldn't even have to think about awaiting the module load. This base class constructor would take care of calling import, and obviously it could deal with IAsyncDisposable too. Then the developer's own code could be reduced to something like:

public class ExampleJsInterop : JSModuleBase
{
    public ExampleJsInterop(IJSRuntime jsRuntime)
        : base(jsRuntime, "./_content/Company.RazorClassLibrary1/exampleJsInterop.js")
    {
    }

    public ValueTask<string> Prompt(string message)
        => InvokeAsync<string>("showPrompt", message);
}

Not suggesting we do that right now, but if customers start using JS modules a lot, it might be worth thinking as a feature for 6.0.

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Sep 4, 2020
@SteveSandersonMS SteveSandersonMS added this to the 5.0.0-rc2 milestone Sep 4, 2020
@SteveSandersonMS SteveSandersonMS requested a review from a team September 4, 2020 11:28
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 4, 2020
public ExampleJsInterop(IJSRuntime jsRuntime)
{
moduleTask = jsRuntime.InvokeAsync<IJSObjectReference>(
"import", "./_content/Company.RazorClassLibrary1/exampleJsInterop.js").AsTask();
Copy link
Member

Choose a reason for hiding this comment

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

This is so nice!

@javiercn
Copy link
Member

javiercn commented Sep 4, 2020

I've made it an IAsyncDisposable because you could think of it as a good practice to remind people that IJSObjectReference instances are IAsyncDisposable. In practice, as long as ExampleJsInterop is consumed as a scoped or singleton instance, there would be no need ever to dispose the lazy-loaded JS module so it's kind of redundant, but I think we should do it like I have here in order to be more risk averse and not set a bad example that disposability can be ignored.

I agree

Not suggesting we do that right now, but if customers start using JS modules a lot, it might be worth thinking as a feature for 6.0.

This is something I've been giving some thought to with regards to things like prerendering. Doing things like that in the constructor is problematic, so we should think of ways of improving that in the future.

Right now we tell you to do JS interop within OnAfterRender and we "bail out" after that, but I've come up with situations where that is just not enough (like when using DI to inject the JS runtime or when you want to do something during OnInitializedAsync). I think what we have for 5.0 is great, but we should revisit some of these things in 6.0 to make folks more successful.

As I said, it looks great for 5.0

@SteveSandersonMS
Copy link
Member Author

This is something I've been giving some thought to with regards to things like prerendering. Doing things like that in the constructor is problematic, so we should think of ways of improving that in the future.

That's a really good point. I've updated the implementation to use Lazy<T> so it's now compatible with prerendering.

It's a bit more boilerplate code, so adds further to the argument for having a base class in the future.

@SteveSandersonMS SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label Sep 4, 2020
@ghost
Copy link

ghost commented Sep 4, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Sep 4, 2020

@Pilchie Can this be approved for RC2?

Base automatically changed from javiercn/css-isolation-follow-ups to release/5.0-rc2 September 4, 2020 17:54
@Pilchie Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 4, 2020
@Pilchie
Copy link
Member

Pilchie commented Sep 4, 2020

Approved for .NET 5.0 RC2, but has a conflict that needs to be resolved.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/update-rcl-to-use-5.0-features branch from 7a0a1f1 to 6bc0978 Compare September 7, 2020 11:15
@SteveSandersonMS
Copy link
Member Author

@mkArtakMSFT Ready to merge.

@mkArtakMSFT mkArtakMSFT merged commit 9f5276d into release/5.0-rc2 Sep 9, 2020
@mkArtakMSFT mkArtakMSFT deleted the stevesa/update-rcl-to-use-5.0-features branch September 9, 2020 15:25
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 area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants