Add support for updatedTypes in metadata updates #55347
Conversation
dotnet-issue-labeler
bot
added
the
area-blazor
|
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! |
| @@ -43,7 +43,7 @@ internal static async Task InitializeAsync() | |||
| /// For framework use only. | |||
| /// </summary> | |||
| [JSInvokable(nameof(ApplyHotReloadDelta))] | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes) | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes, int[] updatedTypes) | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -43,7 +43,7 @@ internal static async Task InitializeAsync() | |||
| /// For framework use only. | |||
| /// </summary> | |||
| [JSInvokable(nameof(ApplyHotReloadDelta))] | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes) | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes, int[] updatedTypes) | |||
There was a problem hiding this comment.
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.
| Blazor._internal.applyHotReload = (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes: number[]) => { | ||
| dispatcher.invokeDotNetStaticMethod('Microsoft.AspNetCore.Components.WebAssembly', 'ApplyHotReloadDelta', id, metadataDelta, ilDelta, pdbDelta, updatedTypes); |
There was a problem hiding this comment.
Per my other comment:
| Blazor._internal.applyHotReload = (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes: number[]) => { | |
| dispatcher.invokeDotNetStaticMethod('Microsoft.AspNetCore.Components.WebAssembly', 'ApplyHotReloadDelta', id, metadataDelta, ilDelta, pdbDelta, updatedTypes); | |
| Blazor._internal.applyHotReload = (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes?: number[]) => { | |
| dispatcher.invokeDotNetStaticMethod('Microsoft.AspNetCore.Components.WebAssembly', 'ApplyHotReloadDelta', id, metadataDelta, ilDelta, pdbDelta, updatedTypes ?? null); |
| @@ -89,7 +89,7 @@ export interface IBlazor { | |||
| } | |||
|
|
|||
| // APIs invoked by hot reload | |||
| applyHotReload?: (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined) => void; | |||
| applyHotReload?: (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes: number[]) => void; | |||
There was a problem hiding this comment.
| applyHotReload?: (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes: number[]) => void; | |
| applyHotReload?: (id: string, metadataDelta: string, ilDelta: string, pdbDelta: string | undefined, updatedTypes?: number[]) => void; |
Add support for
updatedTypesin metadata updatesDescription
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