Skip to content

Remove await for WASM platform.callEntryPoint#31978

Merged
mkArtakMSFT merged 1 commit intorelease/6.0-preview4from
taparik/removeAwaitOnWASMCallEntryPoint
Apr 21, 2021
Merged

Remove await for WASM platform.callEntryPoint#31978
mkArtakMSFT merged 1 commit intorelease/6.0-preview4from
taparik/removeAwaitOnWASMCallEntryPoint

Conversation

@TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented Apr 20, 2021 •
edited
Loading

Description

Regression introduced via: #31769

  • On Blazor Server, Blazor.start returns a promise that resolves when the .NET code starts up and the app becomes interactive
  • On WebAssembly:
    • Prior to Remove Entrypoint Invoker #31769, Blazor.start behaves the same as server (the promise resolves when the app becomes interactive)
    • Since Remove Entrypoint Invoker #31769, Blazor.start returns a promise that doesn’t resolve until the .NET app exits, which typically means never, and hence prevents its usage for triggering post-.NET startup logic

This PR is meant to revert that specific bit of logic, so that we no longer await the platform.callEntryPoint function and thus Blazor.start returns a promise that resolves when the .NET code starts up and the app becomes interactive. This now matches the behavior from 6.0-preview3.

Customer Impact

Without this PR we inadvertently introduce a breaking change where the Blazor.start promise doesn't resolve until the .NET app exits. This prevents its usage for triggering post-.NET startup logic.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]:

Regression from 6.0-preview3
Regressed by: #31769

Risk

  • High
  • Medium
  • Low

[Justify the selection above]
Returning the await logic to what it was prior to #31769 which was introduced in Preview 4.

Verification

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Addresses #31971

@TanayParikh TanayParikh requested a review from a team as a code owner April 20, 2021 15:55
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Apr 20, 2021
@TanayParikh TanayParikh mentioned this pull request Apr 20, 2021
Closed
@mkArtakMSFT mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Apr 20, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

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.

@mkArtakMSFT mkArtakMSFT added this to the 6.0-preview4 milestone Apr 20, 2021
@TanayParikh TanayParikh removed the Servicing-consider Shiproom approval is required for the issue label Apr 20, 2021
pranavkm
pranavkm approved these changes Apr 20, 2021
View reviewed changes
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.

Changes look fine. As part of merging this in to main, could we add a test for this? I was under the impression that we had end-to-end coverage for all of the startup behavior.

@SteveSandersonMS SteveSandersonMS self-requested a review April 20, 2021 17:01
SteveSandersonMS
SteveSandersonMS approved these changes Apr 20, 2021
View reviewed changes
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.

Nice one!

+1 on the E2E test if possible, though if it ends up being complicated for some reason we could proceed with out it for now.

@TanayParikh
Copy link
Contributor Author

TanayParikh commented Apr 20, 2021 •
edited
Loading

Did the manual validation by running the GlobalizationWasmApp test app. Will add the E2E tests when merging into main. Thanks for the reviews.

@TanayParikh TanayParikh added the Servicing-consider Shiproom approval is required for the issue label Apr 20, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

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.

TanayParikh added a commit that referenced this pull request Apr 20, 2021
@TanayParikh
Remove await for WASM platform.callEntryPoint
ea136d0 
Also added an E2E test.

Addresses #31971

Insertion into preview 4: #31978
@TanayParikh TanayParikh mentioned this pull request Apr 20, 2021
Merged
@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 21, 2021
@mkArtakMSFT mkArtakMSFT merged commit ba7142e into release/6.0-preview4 Apr 21, 2021
@mkArtakMSFT mkArtakMSFT deleted the taparik/removeAwaitOnWASMCallEntryPoint branch April 21, 2021 17:33
TanayParikh added a commit that referenced this pull request Apr 21, 2021
@TanayParikh
Remove await for WASM platform.callEntryPoint (#31997)
5d8525c 
* Remove await for WASM `platform.callEntryPoint`

Also added an E2E test.

Addresses #31971

Insertion into preview 4: #31978

* Update src/Components/test/E2ETest/Tests/StandaloneAppTest.cs
3dots pushed a commit to 3dots/aspnetcore-Web.JS that referenced this pull request Feb 19, 2024
@TanayParikh
Remove await for WASM platform.callEntryPoint (#31997)
67d0919 
* Remove await for WASM `platform.callEntryPoint`

Also added an E2E test.

Addresses #31971

Insertion into preview 4: dotnet/aspnetcore#31978

* Update src/Components/test/E2ETest/Tests/StandaloneAppTest.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

+1 more reviewer

@pranavkm pranavkm pranavkm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue

Projects

None yet

Milestone

6.0-preview4

Development

Successfully merging this pull request may close these issues.

4 participants

@TanayParikh @pranavkm @SteveSandersonMS @mkArtakMSFT