Skip to content

[Blazor] JS root component registration #37180

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

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Oct 1, 2021

Description

Delays the execution of afterStarted until the runtime has been initialized so that customers can invoke methods within the Blazor instance.

Fixes #37074

Customer Impact

This is feedback that we got in the RC where customers where not able to use two of the features that we built (JS Root Components and JS Initializers) together because the blazor webassembly host wasn't ready at the time.

It prevents customers for using JS initializers to initialize their component libraries in JS effectively, which is one of the goals the feature was designed for.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

We have end to end tests in this area, and I've verified manually that the call is now emitted at the right time.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Oct 1, 2021
@javiercn javiercn marked this pull request as ready for review October 1, 2021 15:36
@javiercn javiercn requested a review from a team as a code owner October 1, 2021 15:36
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

This makes sense, but it'd odd that this wasn't done in the initial implementation. Can we have @SteveSandersonMS sign off on this in case he's aware of a reason this was separately handled?

@SteveSandersonMS
Copy link
Member

it'd odd that this wasn't done in the initial implementation

It's the same as for any JS interop call. It's the developer's responsibility to wait until their .NET application is ready before making calls into it from outside.

People can do this using Blazor.start, or another easy pattern is to place a JS interop call from .NET to JS inside Program.cs, e.g.:

await JS.InvokeVoidAsync("doMyJSInitialization");

// alternatively:
await JS.InvokeVoidAsync("import", "./myJsInitializationModule.js");

I'd err on the side of not changing this as a special case for JS root components based only on one customer request, especially given that they also said:

This seems low priority to me, since we do have a solution already, but I do think it would be helpful.

Does that sound reasonable @javiercn?

@javiercn
Copy link
Member Author

javiercn commented Oct 1, 2021

It's the same as for any JS interop call. It's the developer's responsibility to wait until their .NET application is ready before making calls into it from outside.

The issue here is not that the .NET application is not ready, but rather that the host is not yet built (on the .NET side).

People can do this using Blazor.start, or another easy pattern is to place a JS interop call from .NET to JS inside Program.cs, e.g.:

I don't think that's the case, since the promise returned from start doesn't guarantee that the host has been built?

I think more in general, that people is calling a method on the Blazor object within the result from "Blazor.start" or "afterStarted" (which is virtually the same, since it's the last thing that happens during initialization) and at that point the functions in the Blazor object should be ready to accept being called.

I think the fix is simple and that there is no technical reason for us not to internally wait until the framework setups interop internally (as the fix proposes).

@SteveSandersonMS
Copy link
Member

I think the fix is simple and that there is no technical reason for us not to internally wait until the framework setups interop internally (as the fix proposes).

I know we can do it, and I see the fix is simple. My concern is about the wider pattern. Other cases of APIs people can call from JS and might do before the app starts up include:

  • JS interop (DotNet.invokeMethodAsync and DotNet.invokeAsync)
  • Blazor.navigateTo

There may be others in the future. Should they all wait for the runtime to start? Should it be a random subset of them that do? By adding this behavior to Blazor.rootComponents.add, it becomes more confusing when people have to reason about which of the others will also have this behavior.

That was my reasoning before anyway. After more consideration, I guess some APIs are agnostic to whether the runtime has started (e.g., Blazor.registerCustomEventType) so maybe it's false to act as if all these APIs have the same startup semantics. After more consideration, I'm fine with changing this.

@SteveSandersonMS SteveSandersonMS self-requested a review October 1, 2021 19:27
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

The change at https://github.com/dotnet/aspnetcore/pull/37180/files#r720486550 is important; the other comments are just for clarity.

@javiercn javiercn force-pushed the javiercn/fix-js-root-component-registration-race branch from 23d199c to da499ea Compare October 15, 2021 17:32
@javiercn javiercn force-pushed the javiercn/fix-js-root-component-registration-race branch from da499ea to c8a199a Compare January 31, 2022 16:50
@javiercn javiercn merged commit 73ce504 into main Jan 31, 2022
@javiercn javiercn deleted the javiercn/fix-js-root-component-registration-race branch January 31, 2022 19:19
@ghost ghost added this to the 7.0-preview1 milestone Jan 31, 2022
@javiercn javiercn modified the milestones: 7.0-preview1, 7.0-preview2 Jan 31, 2022
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.

Trigger custom event 'BlazorLoaded' so we can hook into the event with JavaScript
3 participants