Add support for updatedTypes in metadata updates

[jeromelaban]

Add support for updatedTypes in metadata updates

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This PR passes along the updateTypes parameter from browserlink, in order for the MetadataUpdateHandler types to be invoked properly.

Related to #52937, corresponding sdk PR: dotnet/sdk#40416

[jeromelaban]

Note, I'm not sure how to test this properly on the CI's side, considering it requires an integration from the SDK as well. Thanks for the reviews!

[jeromelaban]

I'm not sure how the public API policy works here. The JS invoker will likely be able to invoke the method without a type, but there may be a need for a separate method that matches previous versions of VS (which injects the browserlink payload).

[MackinnonBuck]

Rather than having two separate methods, could we allow the updatedTypes parameter to be an int[]?? It seems like this logic should still work even if the updated types are not provided by the scripts injected by the SDK. And we could make updated Blazor._internal.applyHotReload implementation pass updatedTypes ?? null in the call to dispatcher.invokeDotNetStaticMethod.

This is technically a breaking API change, but that should be fine in this case because the API is meant for framework use only (customers should not be relying on it).

If you are able to open the solution in VS, there should be a codefix to update the PublicAPI.*.txt files to reflect the public API changes. If you don't have this set up, I'd be happy to update those files and add the changes to this PR.

[jeromelaban]

Thanks for the review! I was indeed able to run the code fix, it added an entry in the unshipped file.

[MackinnonBuck]

Thanks for the contribution, @jeromelaban!

Note, I'm not sure how to test this properly on the CI's side, considering it requires an integration from the SDK as well. Thanks for the reviews!

We should be able to merge each PR separately without breaking things. If the code in this PR accounts for the possibility that updatedTypes is null, then we should be able to merge it before we merge the change in the SDK.

Have you been able to manually verify the changes from both PRs end-to-end?

[MackinnonBuck]

Rather than having two separate methods, could we allow the updatedTypes parameter to be an int[]?? It seems like this logic should still work even if the updated types are not provided by the scripts injected by the SDK. And we could make updated Blazor._internal.applyHotReload implementation pass updatedTypes ?? null in the call to dispatcher.invokeDotNetStaticMethod.

This is technically a breaking API change, but that should be fine in this case because the API is meant for framework use only (customers should not be relying on it).

If you are able to open the solution in VS, there should be a codefix to update the PublicAPI.*.txt files to reflect the public API changes. If you don't have this set up, I'd be happy to update those files and add the changes to this PR.

[jeromelaban]

@MackinnonBuck I just saw that I missed part of your question. I have not been able to validate the changes end-to-end, as it requires a change that I cannot make in visual studio.

[MackinnonBuck]

It's possible to verify the changes via dotnet watch, although I will admit it's not easy to set up and debug. We'll need someone from the Blazor team (either me or someone else) to validate the change.

@jeromelaban, could you provide some clarity on what the motivating scenario for this fix is (i.e., does it benefit Uno)? That will help us prioritize getting this PR tested and merged. Blazor WebAssembly shouldn't really be impacted by this bug today, since its built-in metadata update handlers don't rely on the updatedTypes argument, which is why I'd like to get a sense on what the external impact of this fix would be. Thank you!

[jeromelaban]

It's possible to verify the changes via dotnet watch, although I will admit it's not easy to set up and debug. We'll need someone from the Blazor team (either me or someone else) to validate the change.

Thanks!

@jeromelaban, could you provide some clarity on what the motivating scenario for this fix is (i.e., does it benefit Uno)? That will help us prioritize getting this PR tested and merged. Blazor WebAssembly shouldn't really be impacted by this bug today, since its built-in metadata update handlers don't rely on the updatedTypes argument, which is why I'd like to get a sense on what the external impact of this fix would be. Thank you!

This is impacting us because of the way we detect updated types to perform selective hot reload.

We need to know which types have been changed in order to change displayed elements specifically based on their types (particularly for C# markup). Currently, we're using a process that scans the whole set of loaded types in the AppDomain in order to find types tagged with CreateNewOnMetadataUpdate. This is is computationally expensive (as we need to heuristically detect changes on the main thread) and this is error prone, as we may miss changes done by the IDE if the compilation takes too long.

[MackinnonBuck]

Thanks for the info @jeromelaban, that helps a lot.

We currently have some other high-priority items that need to be addressed in the short-term, but I'll optimistically slot this into .NET 9 preview 7 for planning.

[MackinnonBuck]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).