Retain DOM nodes during streaming SSR updates #48258
Conversation
ghost
added
the
SteveSandersonMS
force-pushed
the
stevesa/dom-merging
branch
2 times, most recently
from
0bde45a to
e0ae761
Compare
SteveSandersonMS
changed the title
| case Node.ELEMENT_NODE: | ||
| // 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. | ||
| // 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 | ||
| // inserts/deletes to find matches based on key to treat those pairs as 'move' operations. | ||
| return (a as Element).tagName === (b as Element).tagName ? UpdateCost.None : UpdateCost.Infinite; |
There was a problem hiding this comment.
Maybe this question has already been considered before (or maybe a solution is already in the works), so I apologize if there was a conversation I missed!
I could see how comparing the element type is enough to eliminate ambiguity in many cases, but could there also be issues where user-entered state "shifts" when content gets synchronized? For example:
<!-- Original content -->
<input type="text" placeholder="Email"> <!-- '[email protected]' -->
<input type="text" placeholder="Phone #"> <!-- '12345678' -->
<!-- Modified content -->
<input type="text" placeholder="Email"> <!-- '[email protected]' -->
<input type="number" placeholder="Zip"> <!-- '12345678' -->
<input type="text" placeholder="Phone #"> <!-- '' -->In this example, the user-entered phone number becomes the zip code. If we considered the input's "type" attribute as part of the comparison, I think it would resolve this particular example, but the issue would persist if the inserted element was also a "text" input.
I see your comment here about @key - would we consider going further and including sequence numbers as special attributes (at least for input-like HTML elements) and use those in the comparison here?
There was a problem hiding this comment.
That's a really great point. I hadn't considered the unique aspects of input-type elements and how it could go wrong.
Rather than trying to guess the developer's intent, it would be much safer simply to say we treat input like any other element, in that the new content always takes precedence and replaces all old state. So what I plan to do is:
- Update the "update attributes" logic so that it has the same "set special values" logic as we use in
BrowserRenderer.ts(i.e., when assigning avalueorchecked, depending on the element type, we also write it as a property not just an attribute) - Update the test to show that we do still preserve user edits if no attribute change occurred, but we don't preserve it if the value attribute did change
In theory the issue you describe could still occur, but it would be much more of an edge case as it would only occur if the old and new elements had identical value attributes but somehow represented unrelated parts of the UI.
There was a problem hiding this comment.
OK, following this feedback, I've taken an even more conservative approach:
- Uses the
tryApplySpecialPropertylogic that was previously exclusive toBrowserRenderer.ts. That is, it now respects all the special cases where we need to assign input-type element values as properties rather than attributes. This ensures that whenever we update an "attribute", it does overwrite any user edits, making the DOM really synchronized. - When deciding whether an "attribute" is changed, implemented the inverse of the above, so we respect special cases to read back the property values rather than attribute values for the same set of input-type elements. This avoids even the "more of an edge case" scenario I mentioned above, since now we no longer care whether the value attribute has changed - for input-type elements we work in terms of the property instead.
I think this now covers just about everything, although the sheer sophistication of the update logic now makes me strongly suspect we'll eventually uncover even more special cases we will want to handle. What makes me happy enough to proceed anyway is:
- The set of special cases accounted for comes from the existing battle-tested
BrowserRenderer.ts, which we know people have been fine with in production for years - We've still got some preview releases to go
- In the worst case, people can still disable this DOM node retention behavior
One thing in particular that I know won't be ideal is the handling of custom elements that maintain internal state. For those, we're just going to update the attributes when preserving those elements, since we have no way of telling them to reset their internal state or what that would even mean for them. However, the same is true for our interactive rendering approach, so we're not really causing new problems here (hopefully).
This reverts commit aaeed13.
SteveSandersonMS
force-pushed
the
stevesa/dom-merging
branch
from
9411d16 to
ef4d392
Compare
| @@ -0,0 +1 @@ | |||
| workspaces-experimental false | |||
There was a problem hiding this comment.
Unfortunately yes. Without it, it fails in CI due to a yarn restore issue related to Jest and its TypeScript integration.
| Browser.FindElement(By.Id("end-response-link")).Click(); | ||
| Browser.Equal("Finished", () => originalStatusElem.Text); | ||
| Assert.Equal(originalLi.Location, Browser.Exists(By.TagName("li")).Location); | ||
| } |
There was a problem hiding this comment.
There's quite a good amount of logic for dealing with special cases (like inputs, etc. doesn't that warrant some E2E test cases?)
There was a problem hiding this comment.
Discussed separately - the JSDom tests cover things in a fairly E2E way, but agree it would be good to add further true E2E tests for inputs. But I'll prioritize merging this for preview 6 first.
Implements #47258
The majority of this PR is a general DOM synchronization mechanism that will also be used for #48761 and #48762. However in this PR it's only used for streaming SSR updates.
Most of the tests here are using a new Jest test setup that gets reported in CI thanks to some help from @BrennanConroy. That's why there's only one E2E test which is verifying that this mechanism actually gets used in practice in streaming SSR.