PR feedback

src/Components/Web.JS/src/Boot.Server.ts

@@ -8,6 +8,7 @@ import { setCircuitOptions, startServer } from './Boot.Server.Common';
 import { ServerComponentDescriptor, discoverComponents } from './Services/ComponentDescriptorDiscovery';
 import { DotNet } from '@microsoft/dotnet-js-interop';
 import { InitialRootComponentsList } from './Services/InitialRootComponentsList';
+import { JSEventRegistry } from './Services/JSEventRegistry';
 
 let started = false;
 
@@ -19,6 +20,7 @@ function boot(userOptions?: Partial<CircuitStartOptions>): Promise<void> {
 
   setCircuitOptions(userOptions);
 
+  JSEventRegistry.create(Blazor);
   const serverComponents = discoverComponents(document, 'server') as ServerComponentDescriptor[];
   const components = new InitialRootComponentsList(serverComponents);
   return startServer(components);

src/Components/Web.JS/src/Boot.Web.ts

@@ -19,6 +19,7 @@ import { NavigationEnhancementCallbacks, attachProgressivelyEnhancedNavigationLi
 import { WebRootComponentManager } from './Services/WebRootComponentManager';
 import { hasProgrammaticEnhancedNavigationHandler, performProgrammaticEnhancedNavigation } from './Services/NavigationUtils';
 import { attachComponentDescriptorHandler, registerAllComponentDescriptors } from './Rendering/DomMerging/DomSync';
+import { JSEventRegistry } from './Services/JSEventRegistry';
 
 let started = false;
 let rootComponentManager: WebRootComponentManager;
@@ -44,12 +45,15 @@ function boot(options?: Partial<WebStartOptions>) : Promise<void> {
   setWebAssemblyOptions(options?.webAssembly);
 
   rootComponentManager = new WebRootComponentManager(options?.ssr?.circuitInactivityTimeoutMs ?? 2000);
-  const enqueueDispatchEnhancedLoad = createEnhancedLoadDispatcher();
+  const jsEventRegistry = JSEventRegistry.create(Blazor);
 
   const navigationEnhancementCallbacks: NavigationEnhancementCallbacks = {
     documentUpdated: () => {
       rootComponentManager.onDocumentUpdated();
-      enqueueDispatchEnhancedLoad();
+      jsEventRegistry.dispatchEvent('enhancedload', {});
+    },
+    enhancedNavigationCompleted() {
+      rootComponentManager.onEnhancedNavigationCompleted();
     },
   };
 
@@ -77,24 +81,6 @@ function onInitialDomContentLoaded() {
   rootComponentManager.onDocumentUpdated();
 }
 
-// This function ensures that 'enhancedload' only gets invoked once
-// for any synchronous sequence of document updates via SSR.
-function createEnhancedLoadDispatcher() {
-  let isDispatchPending = false;
-
-  return function() {
-    if (isDispatchPending) {
-      return;
-    }
-
-    isDispatchPending = true;
-    setTimeout(() => {
-      isDispatchPending = false;
-      Blazor._internal.dispatchEvent('enhancedload', {});
-    }, 0);
-  };
-}
-
 Blazor.start = boot;
 window['DotNet'] = DotNet;
 

src/Components/Web.JS/src/Boot.WebAssembly.ts

@@ -10,6 +10,7 @@ import { setWebAssemblyOptions, startWebAssembly } from './Boot.WebAssembly.Comm
 import { WebAssemblyComponentDescriptor, discoverComponents } from './Services/ComponentDescriptorDiscovery';
 import { DotNet } from '@microsoft/dotnet-js-interop';
 import { InitialRootComponentsList } from './Services/InitialRootComponentsList';
+import { JSEventRegistry } from './Services/JSEventRegistry';
 
 let started = false;
 
@@ -21,6 +22,7 @@ async function boot(options?: Partial<WebAssemblyStartOptions>): Promise<void> {
 
   setWebAssemblyOptions(options);
 
+  JSEventRegistry.create(Blazor);
   const webAssemblyComponents = discoverComponents(document, 'webassembly') as WebAssemblyComponentDescriptor[];
   const components = new InitialRootComponentsList(webAssemblyComponents);
   await startWebAssembly(components);

src/Components/Web.JS/src/GlobalExports.ts

@@ -18,7 +18,7 @@ import { RootComponentsFunctions } from './Rendering/JSRootComponents';
 import { attachWebRendererInterop } from './Rendering/WebRendererInteropMethods';
 import { WebStartOptions } from './Platform/WebStartOptions';
 import { RuntimeAPI } from 'dotnet';
-import { EventRegistry } from './Services/EventRegistry';
+import { JSEventRegistry } from './Services/JSEventRegistry';
 
 // TODO: It's kind of hard to tell which .NET platform(s) some of these APIs are relevant to.
 // It's important to know this information when dealing with the possibility of mulitple .NET platforms being available.
@@ -34,8 +34,8 @@ export interface IBlazor {
   navigateTo: (uri: string, options: NavigationOptions) => void;
   registerCustomEventType: (eventName: string, options: EventTypeOptions) => void;
 
-  addEventListener: typeof EventRegistry.prototype.addEventListener;
-  removeEventListener: typeof EventRegistry.prototype.removeEventListener;
+  addEventListener?: typeof JSEventRegistry.prototype.addEventListener;
+  removeEventListener?: typeof JSEventRegistry.prototype.removeEventListener;
   disconnect?: () => void;
   reconnect?: (existingConnection?: HubConnection) => Promise<boolean>;
   defaultReconnectionHandler?: DefaultReconnectionHandler;
@@ -49,7 +49,6 @@ export interface IBlazor {
     domWrapper: typeof domFunctions;
     Virtualize: typeof Virtualize;
     PageTitle: typeof PageTitle;
-    dispatchEvent: typeof EventRegistry.prototype.dispatchEvent;
     forceCloseConnection?: () => Promise<void>;
     InputFile?: typeof InputFile;
     NavigationLock: typeof NavigationLock;
@@ -94,13 +93,9 @@ export interface IBlazor {
   }
 }
 
-const eventRegistry = new EventRegistry();
-
 export const Blazor: IBlazor = {
   navigateTo,
   registerCustomEventType,
-  addEventListener: eventRegistry.addEventListener.bind(eventRegistry),
-  removeEventListener: eventRegistry.removeEventListener.bind(eventRegistry),
   rootComponents: RootComponentsFunctions,
   runtime: {} as RuntimeAPI,
 
@@ -113,11 +108,8 @@ export const Blazor: IBlazor = {
     NavigationLock,
     getJSDataStreamChunk: getNextChunk,
     attachWebRendererInterop,
-    dispatchEvent: eventRegistry.dispatchEvent.bind(eventRegistry),
   },
 };
 
-eventRegistry.attachBlazorInstance(Blazor);
-
 // Make the following APIs available in global scope for invocation from JS
 window['Blazor'] = Blazor;

src/Components/Web.JS/src/Rendering/DomMerging/DataPermanentElementSync.ts

@@ -1,23 +1,15 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+const dataPermanentAttributeName = 'data-permanent';
+
 export function isDataPermanentElement(elem: Element): boolean {
-  return elem.hasAttribute('data-permanent');
+  return elem.hasAttribute(dataPermanentAttributeName);
 }
 
-export function areIncompatibleDataPermanentElements(elementA: Element, elementB: Element) {
-  const isDataPermanentA = isDataPermanentElement(elementA);
-  const isDataPermanentB = isDataPermanentElement(elementB);
-
-  if (isDataPermanentA !== isDataPermanentB) {
-    // A 'data permanent' element can't be merged with a 'non-data-permanent' one.
-    return true;
-  }
-
-  if (isDataPermanentA && elementA.id !== elementB.id) {
-    // Data permanent elements with different IDs can't be merged.
-    return true;
-  }
+export function cannotMergeDueToDataPermanentAttributes(elementA: Element, elementB: Element) {
+  const dataPermanentAttributeValueA = elementA.getAttribute(dataPermanentAttributeName);
+  const dataPermanentAttributeValueB = elementB.getAttribute(dataPermanentAttributeName);
 
-  return false;
+  return dataPermanentAttributeValueA !== dataPermanentAttributeValueB;
 }

src/Components/Web.JS/src/Rendering/DomMerging/DomSync.ts

@@ -6,7 +6,7 @@ import { isInteractiveRootComponentElement } from '../BrowserRenderer';
 import { applyAnyDeferredValue } from '../DomSpecialPropertyUtil';
 import { LogicalElement, getLogicalChildrenArray, getLogicalNextSibling, getLogicalParent, getLogicalRootDescriptor, insertLogicalChild, insertLogicalChildBefore, isLogicalElement, toLogicalElement, toLogicalRootCommentElement } from '../LogicalElements';
 import { synchronizeAttributes } from './AttributeSync';
-import { areIncompatibleDataPermanentElements, isDataPermanentElement } from './DataPermanentElementSync';
+import { cannotMergeDueToDataPermanentAttributes, isDataPermanentElement } from './DataPermanentElementSync';
 import { UpdateCost, ItemList, Operation, computeEditScript } from './EditScript';
 
 let descriptorHandler: DescriptorHandler | null = null;
@@ -290,8 +290,6 @@ function domNodeComparer(a: Node, b: Node): UpdateCost {
       // For elements, we're only doing a shallow comparison and don't know if attributes/descendants are different.
       // We never 'update' one element type into another. We regard the update cost for same-type elements as zero because
       // then the 'find common prefix/suffix' optimization can include elements in those prefixes/suffixes.
-      // If an element has the 'data-permanent' attribute, it may have content that should be retained between DOM synchronizations.
-      // To maximize the chance that data permanent elements get matched up correctly, we compare the IDs of each element.
       // TODO: If we want to support some way to force matching/nonmatching based on @key, we can add logic here
       //       to return UpdateCost.Infinite if either has a key but they don't match. This will prevent unwanted retention.
       //       For the converse (forcing retention, even if that means reordering), we could post-process the list of
@@ -300,7 +298,10 @@ function domNodeComparer(a: Node, b: Node): UpdateCost {
         return UpdateCost.Infinite;
       }
 
-      if (areIncompatibleDataPermanentElements(a as Element, b as Element)) {
+      // The two elements must have matching 'data-permanent' attribute values for them to be merged. If they don't match, either:
+      // [1] We're comparing a data-permanent element to a non-data-permanent one.
+      // [2] We're comparing elements that represent two different data-permanent containers.
+      if (cannotMergeDueToDataPermanentAttributes(a as Element, b as Element)) {
         return UpdateCost.Infinite;
       }
 

src/Components/Web.JS/src/Services/EventRegistry.ts → src/Components/Web.JS/src/Services/JSEventRegistry.ts

@@ -6,7 +6,6 @@ import { IBlazor } from '../GlobalExports';
 // The base Blazor event type.
 // Properties listed here get assigned by the event registry in 'dispatchEvent'.
 interface BlazorEvent {
-  blazor: IBlazor;
   type: keyof BlazorEventMap;
 }
 
@@ -15,13 +14,14 @@ export interface BlazorEventMap {
   'enhancedload': BlazorEvent;
 }
 
-export class EventRegistry {
+export class JSEventRegistry {
   private readonly _eventListeners = new Map<string, Set<(ev: any) => void>>();
 
-  private _blazor: IBlazor | null = null;
-
-  public attachBlazorInstance(blazor: IBlazor) {
-    this._blazor = blazor;
+  static create(blazor: IBlazor): JSEventRegistry {
+    const result = new JSEventRegistry();
+    blazor.addEventListener = result.addEventListener.bind(result);
+    blazor.removeEventListener = result.removeEventListener.bind(result);
+    return result;
   }
 
   public addEventListener<K extends keyof BlazorEventMap>(type: K, listener: (ev: BlazorEventMap[K]) => void): void {
@@ -35,27 +35,17 @@ export class EventRegistry {
   }
 
   public removeEventListener<K extends keyof BlazorEventMap>(type: K, listener: (ev: BlazorEventMap[K]) => void): void {
-    const listenersForEventType = this._eventListeners.get(type);
-    if (!listenersForEventType) {
-      return;
-    }
-
-    listenersForEventType.delete(listener);
+    this._eventListeners.get(type)?.delete(listener);
   }
 
   public dispatchEvent<K extends keyof BlazorEventMap>(type: K, ev: Omit<BlazorEventMap[K], keyof BlazorEvent>): void {
-    if (this._blazor === null) {
-      throw new Error('Blazor events cannot be dispatched until a Blazor instance gets attached');
-    }
-
     const listenersForEventType = this._eventListeners.get(type);
     if (!listenersForEventType) {
       return;
     }
 
     const event: BlazorEventMap[K] = {
       ...ev,
-      blazor: this._blazor,
       type,
     };
 

src/Components/Web.JS/src/Services/NavigationEnhancement.ts

@@ -37,6 +37,7 @@ let performingEnhancedPageLoad: boolean;
 
 export interface NavigationEnhancementCallbacks {
   documentUpdated: () => void;
+  enhancedNavigationCompleted: () => void;
 }
 
 export function isPageLoading() {
@@ -250,7 +251,7 @@ export async function performEnhancedPageLoad(internalDestinationHref: string, f
     }
 
     performingEnhancedPageLoad = false;
-    navigationEnhancementCallbacks.documentUpdated();
+    navigationEnhancementCallbacks.enhancedNavigationCompleted();
   }
 }
 

src/Components/Web.JS/src/Services/WebRootComponentManager.ts

@@ -73,6 +73,13 @@ export class WebRootComponentManager implements DescriptorHandler, RootComponent
   }
 
   public onDocumentUpdated() {
+    // Root components may have been added, updated, or removed.
+    this.rootComponentsMayRequireRefresh();
+  }
+
+  public onEnhancedNavigationCompleted() {
+    // Root components may now be ready for activation if they had been previously
+    // skipped for activation due to an enhanced navigation being underway.
     this.rootComponentsMayRequireRefresh();
   }
 

src/Components/Web.JS/test/DomSync.test.ts

@@ -492,6 +492,63 @@ describe('DomSync', () => {
     expect(newDocTypeNode).toBe(origDocTypeNode);
     expect(destination.body.textContent).toBe('Goodbye');
   });
+
+  test('should preserve content in elements marked as data permanent', () => {
+    // Arrange
+    const destination = makeExistingContent(`<div>not preserved</div><div data-permanent>preserved</div><div>also not preserved</div>`);
+    const newContent = makeNewContent(`<div></div><div data-permanent>other content</div><div></div>`);
+    const oldNodes = toNodeArray(destination);
+
+    // Act
+    synchronizeDomContent(destination, newContent);
+    const newNodes = toNodeArray(destination);
+
+    // Assert
+    expect(oldNodes[0]).toBe(newNodes[0]);
+    expect(oldNodes[1]).toBe(newNodes[1]);
+    expect(newNodes[0].textContent).toBe('');
+    expect(newNodes[1].textContent).toBe('preserved');
+  });
+
+  test('should preserve content in elements marked as data permanent by matching attribute value', () => {
+    // Arrange
+    const destination = makeExistingContent(`<div>not preserved</div><div data-permanent="first">first preserved</div>`);
+    const newContent1 = makeNewContent(`<div>not preserved</div><div data-permanent="second">second preserved</div><div data-permanent="first">other content</div>`);
+    const newContent2 = makeNewContent(`<div>not preserved</div><div data-permanent="second">other content</div><div id="foo"></div><div data-permanent="first">other content</div>`);
+    const nodes1 = toNodeArray(destination);
+
+    // Act/assert 1: The original data permanent content is preserved
+    synchronizeDomContent(destination, newContent1);
+    const nodes2 = toNodeArray(destination);
+    expect(nodes1[1]).toBe(nodes2[2]);
+    expect(nodes2[1].textContent).toBe('second preserved');
+    expect(nodes2[2].textContent).toBe('first preserved');
+
+    // Act/assert 2: The new data permanent content is preserved
+    synchronizeDomContent(destination, newContent2);
+    const nodes3 = toNodeArray(destination);
+    expect(nodes2[1]).toBe(nodes3[1]);
+    expect(nodes2[2]).toBe(nodes3[3]);
+    expect(nodes3[1].textContent).toBe('second preserved');
+    expect(nodes3[3].textContent).toBe('first preserved');
+  });
+
+  test('should not preserve content in elements marked as data permanent if attribute value does not match', () => {
+    // Arrange
+    const destination = makeExistingContent(`<div>not preserved</div><div data-permanent="first">preserved</div><div>also not preserved</div>`);
+    const newContent = makeNewContent(`<div></div><div data-permanent="second">new content</div><div></div>`);
+    const oldNodes = toNodeArray(destination);
+
+    // Act
+    synchronizeDomContent(destination, newContent);
+    const newNodes = toNodeArray(destination);
+
+    // Assert
+    expect(oldNodes[0]).toBe(newNodes[0]);
+    expect(oldNodes[1]).not.toBe(newNodes[1]);
+    expect(newNodes[0].textContent).toBe('');
+    expect(newNodes[1].textContent).toBe('new content');
+  });
 });
 
 test('should remove value if neither source nor destination has one', () => {