[Blazor] APIs to preserve content between enhanced navigations #50437

MackinnonBuck · 2023-08-30T19:13:06Z

APIs to preserve content between enhanced navigations

Adds APIs to enable the preservation of DOM content between enhanced navigations.

Description

This PR introduces two new ways to preserve content between enhanced navigations:

The data-permanent HTML attribute
- Elements that contain this attribute will have their content preserved between enhanced page updates. The id attribute on an element marked as data-permanent will be used to match up data permanent elements when performing DOM synchronization.
- Note that the data permanent element itself is still subject to removal via SSR update if the updated region includes the existing data-permanent element. Therefore, the data-permanent element itself should not be added dynamically via JS, (but its content can be, which is the primary use case).
Blazor.registerEnhancedPageUpdateCallback(callback)
- This API can be used in cases where the data-permanent attribute is not sufficient. It lets you get notified when an enhanced page update occurs so changes to the DOM can be made in response.
- Callbacks will be invoked any time the document changes due to an enhanced update, including streaming rendering updates and enhanced form posts. It does not only get involved when a navigation occurs.
- Callbacks do not get invoked automatically upon registration or after the initial page load.

SteveSandersonMS · 2023-08-31T09:01:11Z

The id attribute on an element marked as data-permanent will be used to match up data permanent elements when performing DOM synchronization.

API design question: rather than require people to use id for this (which has other meanings too), would it make more sense for the attribute's value to be the permanence identifier? Example: <div data-permanent="dynamic-cart">. And pros/cons?

SteveSandersonMS · 2023-08-31T09:04:27Z

Blazor.registerEnhancedPageUpdateCallback(callback)

That's great, but seems like you'd also need a way to unregister. To make this feel even more natural, what do you think about copying the DOM event APIs, e.g.,

Blazor.addEventListener('enhancedload', myFunction);
Blazor.removeEventListener('enhancedload', myFunction);

I know this opens the question of "what other events could I listen for". Whether that's a good or bad thing depends on the likelihood of us adding other listenable events in the future.

javiercn · 2023-08-31T13:58:43Z

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

@@ -33,6 +33,7 @@ interface IBlazor {
  navigateTo: (uri: string, options: NavigationOptions) => void;
  registerCustomEventType: (eventName: string, options: EventTypeOptions) => void;

+  registerEnhancedPageUpdateCallback?: (callback: () => void) => { dispose(): void };


We can scope this out to enhanced navigation, but we might also make it more general and have this callback happen after every render update gets applied (enhanced navigation or interactive).

The people that do this through enhanced navigation is going to fall off a cliff the moment they make their component interactive, as the same approach won't work in that case.

javiercn · 2023-08-31T14:03:44Z

I agree with @SteveSandersonMS on the comment about the id.

If we were to use the id attribute, then we shouldn't need to use the data-permanence attribute, but I acknowledge there might be reasons why equating the two is not necessarily useful or desirable (not just because you give something and id you want it preserved, but there are also many cases where that's likely is the case).

So, I would say either we ignore the id and use data-permanence="id" or just use the id, and given the unknowns I am more inclined to go with @SteveSandersonMS recommendation here.

javiercn · 2023-08-31T14:04:45Z

Blazor.registerEnhancedPageUpdateCallback(callback)

That's great, but seems like you'd also need a way to unregister. To make this feel even more natural, what do you think about copying the DOM event APIs, e.g.,
Blazor.addEventListener('enhancedload', myFunction);
Blazor.removeEventListener('enhancedload', myFunction);
I know this opens the question of "what other events could I listen for". Whether that's a good or bad thing depends on the likelihood of us adding other listenable events in the future.

Seems fair. What other things we think we might add in the future here?

SteveSandersonMS · 2023-08-31T14:12:42Z

Callbacks will be invoked any time the document changes due to an enhanced update, including streaming rendering updates and enhanced form posts. It does not only get involved when a navigation occurs.

That's interesting. We haven't really spelled out the scenarios and the reasons why someone would want to react on the JS side when each of the following occurs, and whether they have to be able to differentiate these occurrences:

Enhanced navigation completes excluding streaming (so equivalent to DOMContentLoaded)
Enhanced navigation completes including streaming (no direct equivalent DOM event, as it's something like "all my code is quiescent")
Interactive navigation completes
Each streaming render update occurs
Each interactive render update occurs

I'd be keen to avoid adding lots of extensibility for everything without having a definite rationale and customer demand, as it all restricts us in the future.

What's the smallest set of these we could limit it to, based on the known scenarios?

SteveSandersonMS · 2023-08-31T14:14:09Z

So, I would say either we ignore the id and use data-permanence="id" or just use the id, and given the unknowns I am more inclined to go with @SteveSandersonMS recommendation here.

Sounds good. On naming, I'd lean towards data-permanent rather than data-permanence since it's a more basic word and easier to type, and aligns with Turbo's existing naming so has a bit more recognition.

javiercn · 2023-08-31T14:16:47Z

Callbacks do not get invoked automatically upon registration or after the initial page load.

Is there a reason not to do this? It will require people to have two code paths, one for the first initialization and another one after an update happens. Yes, they can factor the code for max reuse, but it seems like extra work that could be avoided?

javiercn · 2023-08-31T14:19:13Z

That's interesting. We haven't really spelled out the scenarios and the reasons why someone would want to react on the JS side when each of the following occurs, and whether they have to be able to differentiate these occurrences:

I think what it comes down to is that we need to have a crisp understanding of how this works when a component becomes interactive. If developers need to reach out for a different mechanism in those situations, I believe we are failing to solve the issue effectively.

SteveSandersonMS · 2023-08-31T14:21:10Z

I think what it comes down to is that we need to have a crisp understanding of how this works when a component becomes interactive.

Can you clarify more about what you mean? I'm unsure why "component becomes interactive" is central to the problem, since the problem also occurs even without any interactive components.

javiercn · 2023-08-31T14:24:03Z

Can you clarify more about what you mean? I'm unsure why "component becomes interactive" is central to the problem, since the problem also occurs even without any interactive components.

If the API is called once enhanced navigation happens and is used to re-add attributes to an element dynamically, then if in a different circumstance the component is rendered as webassembly/server, that callback won't be triggered when they update the DOM, and the attributes won't necessarily be preserved. So at that point, the dev would need to use intersection observer or similar to re-attach the attributes.

SteveSandersonMS · 2023-08-31T14:29:01Z

if in a different circumstance the component is rendered as webassembly/server, that callback won't be triggered when they update the DOM, and the attributes won't necessarily be preserved.

It's absolutely not supported to mutate interactively-rendered DOM from JS, so I'd definitely not do anything to facilitate that.

MackinnonBuck · 2023-08-31T18:33:37Z

API design question: rather than require people to use id for this (which has other meanings too), would it make more sense for the attribute's value to be the permanence identifier?

I like that a lot! That does seem cleaner than relying on a separate attribute with other meanings.

That's great, but seems like you'd also need a way to unregister. To make this feel even more natural, what do you think about copying the DOM event APIs, e.g.,

There actually is a way to unregister, but I forgot to mention it in my PR comment. You can use the value returned by registerEnhancedPageUpdateCallback() to unregister the callback, e.g.,

const registration = Blazor.registerEnhancedPageUpdateCallback(callback);
// ...
registration.dispose();

However, I do like your suggestion. It feels more JavaScriptey, and it helps enforce a level of consistency for Blazor events we might add in the future.

Taking it further, what if we added custom events on document? So you would do something like:

document.addEventListener('blazorenhancedload', myFunction);
document.removeEventListener('blazorenhancedload', myFunction);

This has the advantage that you don't need to wait until the Blazor object gets defined before you register handlers for these events.

We haven't really spelled out the scenarios and the reasons why someone would want to react on the JS side when each of the following occurs, and whether they have to be able to differentiate these occurrences... What's the smallest set of these we could limit it to, based on the known scenarios?

The main scenario I can think of is applying mutations to the DOM that just got erased by an enhanced update. Assuming we want to support applications mutating any part of the DOM (excluding interactive content), then I believe it follows that any enhanced update has potential to step over a DOM mutation made by the application. The conclusion I reach from there is that each type of enhanced content update should invoke the enhancedload event (so scenarios 1, 2, and 4).

If customers only want to listen to a subset of those scenarios, we could potentially extend this by adding a type argument to the event or introducing new events for more restricted cases (e.g., enhancednavigationend for scenario 2 only).

Callbacks do not get invoked automatically upon registration or after the initial page load.

Is there a reason not to do this? It will require people to have two code paths, one for the first initialization and another one after an update happens. Yes, they can factor the code for max reuse, but it seems like extra work that could be avoided?

I did it this way purely because the initial page load is not an enhanced load. So if the customer really wants to listen only to "enhanced" updates, that initial call might be misleading. If we go with the approach where we add a custom event on document, the customer could just do:

document.addEventListener('blazorenhancedload', myFunction);
document.addEventListener('DOMContentLoaded', myFunction);

Does that seem okay?

javiercn · 2023-09-01T09:19:45Z

Taking it further, what if we added custom events on document? So you would do something like:

This will break the second there are two Blazor instances on the page.

MackinnonBuck · 2023-09-01T21:32:39Z

This will break the second there are two Blazor instances on the page.

I was thinking this would be fine since the main use case is adding content back to the page if it got removed (no matter "who" removed it). But I can see the argument for this being problematic if we want to add "Blazor instance specific" events.

javiercn · 2023-09-04T10:42:11Z

I was thinking this would be fine since the main use case is adding content back to the page if it got removed (no matter "who" removed it). But I can see the argument for this being problematic if we want to add "Blazor instance specific" events.

I'd rather we avoid adding more global state to the page, as it makes that scenario harder in the future.

MackinnonBuck · 2023-09-05T22:51:01Z

I've just updated the API so the usage is now:

Blazor.addEventListener('enhancedload', myFunction);
Blazor.removeEventListener('enhancedload', myFunction);

The argument to the registered listener is an object with the following structure:

{
  blazor: typeof Blazor,
  type: 'enhancedload',
}

The enhancedload event gets dispatched after all SSR updates for the current event cycle have completed.

If the primary purpose of the enhancedload event is to apply changes to the DOM that got reverted by an enhanced page update, then I don't see a need to provide additional info to event listeners about what type of SSR update occurred. However, I could foresee a scenario where the developer wants some non-DOM-mutating JS logic to run specifically after an enhanced navigation occurs (to simulate a new <script> executing or DOMContentLoaded firing), so that a page's initialization logic runs regardless of how the page was navigated to.

If we anticipate that's a distinction that needs to be made, we could modify the event listener's parameter to have the following shape:

{
  blazor: typeof Blazor,
  type: 'enhancedload',
  kind: 'navigationstart' | 'navigationend' | 'update'
}

...where:

'navigationstart' indicates that the first SSR update has occurred following a page navigation
'navigationend' indicates that the HTTP response for the navigation has completed and all its SSR updates have applied
'update' indicates that an SSR update unrelated to navigation has occurred

Thoughts about this?

SteveSandersonMS · 2023-09-11T10:32:00Z

The data-permanent HTML attribute
Elements that contain this attribute will have their content preserved between enhanced page updates

In terms of how we communicate/document this, that sounds slightly misleading because we try to preserve all DOM elements (on a best-effort basis), not just these ones. Am I right to say a more accurate phrasing would be:

Elements with this attribute will not be synchronized during enhanced page updates. These elements and their subtrees will be left entirely unchanged, no matter how the corresponding new element differs. However, the whole element will be removed if there is no corresponding element in the new content.

On reading the algorithm, I see that data-permanent is effective only if the corresponding element in the new content is in a matchable location. For example you can't move a "permanent" element from <head> into <body> as that's never matchable.

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

SteveSandersonMS · 2023-09-11T10:42:12Z

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

+    setTimeout(() => {
+      isDispatchPending = false;
+      Blazor._internal.dispatchEvent('enhancedload', {});
+    }, 0);


setTimeout(..., 0) always sets off alarms since it means the caller loses the ability to do anything synchronous or even run any logic after this callback, without reverting to even more problematic tricks like setTimeout(..., 1) and so on.

Not saying it's necessarily wrong since maybe this really is the only/cleanest way to achieve the goal. But could you clarify why SSR will do things multiple times synchronously, and whether we could instead make SSR responsible for only triggering this once at the end of its process?

If this comes down to "a single chunk of HTTP traffic may include multiple <blazor-ssr> blocks" then sure that's true, but they may arrive arbitrarily quickly anyway, so it might not be particularly useful to distinguish "those that arrive synchronously together" from those who don't. What difference would it make if we fired the event for each one, whether or not there are others in the same synchronous block? The recipient of the event can still debounce if it needs to for its own reasons.

Yeah, I think this is something that the server should tell the JS code, rather than relying on heuristics. Don't we insert a frame at the end of the response?

Not saying it's necessarily wrong since maybe this really is the only/cleanest way to achieve the goal. But could you clarify why SSR will do things multiple times synchronously, and whether we could instead make SSR responsible for only triggering this once at the end of its process?

We currently invoke this callback:

After the initial page load

After the initial content is received during enhanced navigation

After the document completes a streaming response update

After the enhanced navigation completes

This might seem redundant, but it enables reacting to "quiescence". Blazor itself uses this so that new components get activated only when enhanced navigation completes.

Given that the public event ('enhancedload') doesn't distinguish between these four cases, the setTimeout() mechanism was used to prevent, for example, the callback being invoked after receiving initial enhanced navigation content, then immediately again if there's no additional streaming content.

But I do agree that the recipient can debounce these events if it needs to, and not having the setTimeout() also allows us to extend this in the future if we expose which of the four cases resulted in the callback getting invoked (maybe someone wants to listen explicitly for the "navigation end" callback, and deduplicating this event results in that callback getting shadowed by the "initial load" callback).

Edit: So the approach I plan on taking is just removing the setTimeout() and accepting the fact that there might be "duplicate" dispatches. Does that sound good?

I've actually just updated this so the fourth case ("after the enhanced navigation completes") does not dispatch the 'enhancedload' event, because it doesn't signal that changes to the DOM were made in that case. In the future, we could add a navigationend event to allow developers to react to that case.

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

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

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

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

SteveSandersonMS · 2023-09-11T10:57:11Z

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

+interface BlazorEvent {
+  blazor: IBlazor;
+  type: keyof BlazorEventMap;
+}


This is interesting. If I'm reading correctly, your use of blazor here is for forward compatibility with some possible future where we pass the Blazor instance to the callback?

Presumably this is to handle the multiple-Blazor case.

But if so, wouldn't that already be covered by having a different event registration call for each Blazor instance? That would be more compatible, since the different Blazor instances could be of different versions and hence have completely different APIs. Or is there a different reason for doing this?

If it's for internal use only (and I see you used Omit below, perhaps with that in mind) then it would certainly be fine either way.

I agree, if this is for an event, I don't see why we would have blazor as an arg, given that you know where you are registering.

The other bit (which I am a bit unsure is whether this should have a similar shape as a https://developer.mozilla.org/en-US/docs/Web/API/Event and have some of it's behaviors, like this being associated with the event in the handler, and so on.

This is interesting. If I'm reading correctly, your use of blazor here is for forward compatibility with some possible future where we pass the Blazor instance to the callback?

Yeah, that's correct. I added initially added this when taking the document.addEventListener approach (instead of the current approach, which is Blazor.addEventListener), so that developers could detect which Blazor instance emitted the event. But since the event is now getting registered on the Blazor instance itself, it's probably fine that we remove this.

The other bit (which I am a bit unsure) is whether this should have a similar shape as a https://developer.mozilla.org/en-US/docs/Web/API/Event

Couldn't we extend this API in the future in a non-breaking manner to emulate the shape of DOM events a bit more? Or is this something you think we need to decide on now?

Couldn't we extend this API in the future in a non-breaking manner to emulate the shape of DOM events a bit more? Or is this something you think we need to decide on now?

Sure, I'd be fine with treating only the declared API as supported, and undocumented things like "the value of this" being open for change in the future.

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

SteveSandersonMS · 2023-09-11T11:12:21Z

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

+    return true;
+  }
+
+  if (isDataPermanentA && elementA.id !== elementB.id) {


I thought we were changing the rule not to use id, but instead use the values of the data-permanent attributes to control this.

I'd also be happy with some rule like:

If data-permanent is present:

If its value is an empty-string (like it is for valueless attributes, e.g., <div data-permanent>, bearing in mind that the valueless is supported for data-enhance)

If the element has an ID, use that as the data permanence value

Else use the fixed string "" as the data permanence value, being sure not to treat it as false

Else use the attribute value as the data permanence value

TBH I'd also be happy with not using element.id at all, since it's yet another avoidable thing to document and support forever. This is based on the diff algorithm not matching on a global basis, and so it generally is going to behave as desired in many cases if you simply have data-permanent with no attribute value.

What do you think?

I think relying on the data-permanent attribute value and nothing else (ignoring element.id) sounds good. I've just updated the PR accordingly.

SteveSandersonMS · 2023-09-11T11:20:29Z

If we anticipate that's a distinction that needs to be made, we could modify the event listener's parameter to have the following shape:

{
blazor: typeof Blazor,
type: 'enhancedload',
kind: 'navigationstart' | 'navigationend' | 'update'
}

Good point, thanks for raising it!

I would strongly lean towards not taking guesses about what people might want until we know, and hence not add these other kinds. For forward compatibility, there are two ways we could go:

We could add kind: 'update' right now (presuming that's the variant you implemented so far) so that in the future if we want to add other kinds, we could argue it's nonbreaking. We would have to claim that people should have been checking for kind: update and no-opping for other kinds. However that's a dubious strategy since if there's only one kind right now, people could be forgiven for not checking for it, and then if we do add other kinds later it would break them in practice.
We could expect that in the future, we would use different event names for "navigation start" and "navigation end". That's much safer, and also means we don't have to define "kind" right now.

I think option 2 is easier to justify but if you're really keen on option 1 that's a possibility.

javiercn · 2023-09-12T13:23:13Z

We could expect that in the future, we would use different event names for "navigation start" and "navigation end". That's much safer, and also means we don't have to define "kind" right now.

I'm more inclined to go with option 2, that's more in line with how events work on the DOM. The only one that I believe does something like 1 is the mutation observer API.

MackinnonBuck · 2023-09-14T00:02:21Z

Thanks for all the feedback, @SteveSandersonMS and @javiercn! I plan to finish addressing your feedback tomorrow.

MackinnonBuck · 2023-09-14T20:05:50Z

We could expect that in the future, we would use different event names for "navigation start" and "navigation end". That's much safer, and also means we don't have to define "kind" right now.

I'm more inclined to go with option 2, that's more in line with how events work on the DOM. The only one that I believe does something like 1 is the mutation observer API.

That sounds good to me. I've updated the implementation to only dispatch the 'enhancedload' event when changes to the DOM are made (not when enhanced navigation completes), so that should allow us to extend this in the future.

SteveSandersonMS

Looks flawless.

Somehow we have to track the need to document Blazor.addEventListener and the enhancedload event. Would you be able to file a doc issue? You could either provide a summary of the functionality for @guardrex to work from, or not (and he will likely pursue you for the details later).

Depends on dotnet/aspnetcore#50437. Addresses dotnet/aspnetcore#49144. | Scenario | Validated | |---|---| | Statically server rendered page | ✅ | | Interactive server rendered page | ✅ | | Page with stream rendering enabled | ✅ | | Interactive client rendered page | ❌ (dotnet/aspnetcore#50765) | | Statically server rendered page with interactive components (all render modes) | ✅ |

ghost · 2023-11-26T18:25:26Z

@MackinnonBuck, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

MackinnonBuck requested a review from a team as a code owner August 30, 2023 19:13

dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Aug 30, 2023

MackinnonBuck force-pushed the mbuck/preserve-dom branch from 2434163 to fe4021c Compare August 30, 2023 19:19

javiercn reviewed Aug 31, 2023

View reviewed changes