Skip to content

Conversation

@hakenr
Copy link
Member

@hakenr hakenr commented Nov 29, 2024

Even though repeated calls to AssemblyLoader.LoadAssembliesAsync() don't download the assembly multiple times or throw exception:

  • They still trigger asynchronous calls to JavaScript.
  • The <Navigating> content is rendered in the meantime, causing unnecessary UI flickering when navigating to the lazy-loaded area repeatedly.
  • The lazyLoadedAssemblies collection gets populated with duplicate entries.

Therefore, lazy-loading should be guarded with an "already loaded" flag.


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/webassembly-lazy-load-assemblies.md aspnetcore/blazor/webassembly-lazy-load-assemblies

@hakenr hakenr requested a review from guardrex as a code owner November 29, 2024 00:42
@guardrex guardrex self-assigned this Nov 29, 2024
@guardrex guardrex requested a review from a team December 1, 2024 20:04
@guardrex
Copy link
Collaborator

guardrex commented Dec 1, 2024

🤔

This wasn't part of Safia's original bits for the feature ...

dotnet/aspnetcore#23290

... and it never made its way into the test component (but they might want to add it after they see what you wrote) ...

https://github.com/dotnet/aspnetcore/blob/main/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithLazyAssembly.razor

... and it didn't come up for discussion on the docs issue or PR ...

I've added the Blazor engineering group to see if anyone is free to take a look at this. If we don't hear back within a couple of days, I'll reach out offline to get someone on here.

@SteveSandersonMS
Copy link
Member

Adding this guard here is mainly beneficial for the sample code, since otherwise (like @hakenr mentions) it ends up duplicating data. That's not an issue in the product itself; it's just a quirk of the sample code in the docs.

Whether or not people want to use a guard clause to ensure they don't see any flash of "loading..." content is up to the app developer.

Altogether the changes suggested here are good and I think they should be added to the docs, as it gives readers more of a sense of things they might want to do to get good end-to-end behavior in their own apps.

if (args.Path == "robot")
if ((args.Path == "robot") && !grantImaharaRobotControlsAssemblyLoaded)
{
grantImaharaRobotControlsAssemblyLoaded = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT question on the location of this line ...

Because the component survives an exception on the next two lines, does it make sense to move this line (grantImaharaRobotControlsAssemblyLoaded = true;) to the end after the lazyLoadedAssemblies.AddRange(assemblies); line (i.e., it's only true if we know those two lines executed)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to go ahead and move the line down to get this in. I'm about to shoot a bunch of PRs into the live branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

@guardrex Neither of the two alternatives is perfect. I usually prefer to set the flag before an awaited operation - because with the await there, another navigation might occur while waiting for the assembly to load, leaving the flag still false for the second run.

In this case, since the awaited assembly load won’t cause a critical failure if called repeatedly, I agree that your version is better.

Copy link
Collaborator

@guardrex guardrex left a comment

Choose a reason for hiding this comment

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

🎉

@guardrex guardrex enabled auto-merge (squash) December 12, 2024 12:23
@guardrex guardrex merged commit 9092b76 into dotnet:main Dec 12, 2024
3 checks passed
@hakenr hakenr deleted the patch-44 branch December 12, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants