Refactor RehydrateBuilder to be more robust #988

rwjblue · 2019-11-05T22:41:07Z

Given this template:

<p>{{#if true}}<div></div>{{/if}}</p>

The browser auto-corrects it to:

<p></p><div></div><p></p>

When the rehydration builder runs against this, it throws an error (and fails to continue rehydrating) leaving the app in a broken state.

This PR refactors a number of things about how RehydrateBuilder recovers from errors like the one mentioned above, it also fixes some fundamental issues with rehydration of {{in-element}} usage (which was previously broken).

After this refactor, the act of rehydration should never error (leaving the app in a horribly broken state). At worst, whatever amount of SSR'ed content has already been processed when a mismatch occurs will remain stable and the remainder of the content will be cleared just before we set up the new / correct DOM from the initial render.

Paired with @krisselden on most of the fixes / refactors in this PR.

TODO:

Add failing test case for Refactor RehydrateBuilder to be more robust #988 (comment)

rwjblue · 2019-11-06T23:26:30Z

CI is failing on IE11 for bizarro ways because IE11 doesn't insert an end </p> the same way modern browsers do.

With IE11, the following <p>hello <div>world</div></p> is "corrected" to <p>hello <div>world!</div><p></p>, but on modern browsers we get <p>hello </p><div>world!</div><p></p>. 🙀

scalvert · 2019-11-06T23:41:12Z

👍

See glimmerjs/glimmer-vm#988

rwjblue · 2019-11-08T02:49:11Z

packages/@glimmer/integration-tests/test/initial-render-test.ts

@@ -516,7 +545,7 @@ class Rehydration extends AbstractRehydrationTests {
      remoteParent: clientRemoteParent,
      remoteChild: clientRemoteChild,
    });
-    this.assertRehydrationStats({ nodesRemoved: 2 });
+    this.assertRehydrationStats({ nodesRemoved: 0 });


This PR fixes rehydration for in-element, that is why this goes to expect no removed nodes.

rwjblue · 2019-11-08T02:50:49Z

packages/@glimmer/integration-tests/test/initial-render-test.ts

-      toInnerHTML(clientRemote),
-      '<prefix></prefix><suffix></suffix><inner>Wat Wat</inner>'
-    );
+    this.assertRehydrationStats({ nodesRemoved: 2 });


Using {{in-element without passing insertBefore as a named argument always means "clear this element".

https://github.com/emberjs/rfcs/blob/master/text/0287-promote-in-element-to-public-api.md#small-proposed-changes

https://github.com/glimmerjs/glimmer-vm/pull/918/files

chadhietala

Overall looks good, just a couple of questions and a nit.

chadhietala · 2019-11-08T14:12:50Z

packages/@glimmer/integration-tests/test/initial-render-test.ts

+// `window.ActiveXObject` is "falsey" in IE11 (but not `undefined` or `false`)
+// `"ActiveXObject" in window` returns `true` in all IE versions
+// only IE11 will pass _both_ of these conditions
+const isIE11 = !(window as any).ActiveXObject && 'ActiveXObject' in window;


It would be good to annotate this stuff so it's easy to cleanup in a #worldwithoutIE11.

Happy to! How do you think I should annotate?

chadhietala · 2019-11-08T14:18:03Z

packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts

-  private blockDepth = 0;
-
-  // private candidate: Option<SimpleNode> = null;
+  blockDepth = 0;


Is the only reason why this has to be public is to enforce the type on L101?

Yes, if it is private the "special" types that we added for pushElement won't work. The real issue is that NewElementBuilder (our super class) invokes this.pushElement during RehydrateBuilder's super() call which means our class fields haven't ran yet (and therefore this.blockDepth is undefined still). So we tried to make the types such that it was clearer that the this within that method was "weird" (to avoid future mistakes).

packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts

kratiahuja · 2019-11-08T18:25:13Z

👍

Given this template: ```hbs <p>{{#if true}}<div></div>{{/if}}</p> ``` The browser auto-corrects it to: ```html <p></p><div></div><p></p> ``` When the rehydration builder runs against this, it throws an error (and fails to continue rehydrating) leaving the app in a broken state.

Co-authored-by: Kris Selden <kselden@linkedin.com>

packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts

Ensure that updates of `currentCursor.candidate` go through the custom setter for `candidate`. This fixes a number of issues caused by having the wronge `currentCursor.nextSibling` (and therefore not properly rehydrating). Also ensures that `in-element` _can actually_ rehydrate. Prior to this commit we would (nearly universally) clear the target contents. See updated test cases ensuring `0` nodes removed for samples. Co-authored-by: Kris Selden <kselden@linkedin.com>

Co-authored-by: Kris Selden <kselden@linkedin.com>

Previously, we would clear to the cursors openBlockDepth but if rehydration is disabled, the currentCursor.openBlockDepth is not updated as we progress through additional `__openBlock` invocations (due to short curcuit returning). Instead of using the cursors notion of openBlockDepth we use the local `this.blockDepth` value (which is always maintained properly). Co-authored-by: Kris Selden <kselden@linkedin.com>

addressed issue

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from fa591b0 to 4af59c4 Compare November 6, 2019 22:59

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from 4af59c4 to 55b7825 Compare November 7, 2019 02:08

rwjblue added a commit to rwjblue/-demo-disabling-rerendering-in-ssr-mode that referenced this pull request Nov 7, 2019

Refactor the monkey patch to match implementation.

101516c

See glimmerjs/glimmer-vm#988

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from e5793cf to 55b7825 Compare November 7, 2019 17:00

rwjblue mentioned this pull request Nov 7, 2019

Add a set of "chaos monkey" tests for rehydration. #991

Merged

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch 2 times, most recently from c7a9e4c to a9ea31e Compare November 8, 2019 02:46

rwjblue commented Nov 8, 2019

View reviewed changes

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from a9ea31e to 00ce6ef Compare November 8, 2019 03:12

rwjblue changed the title ~~Failing rehydration test for <p>{{#if true}}<div></div>{{/if}}</p>~~ Refactor RehydrateBuilder to be more robust Nov 8, 2019

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from 00ce6ef to 76442d9 Compare November 8, 2019 03:30

rwjblue requested review from chadhietala and tomdale November 8, 2019 03:43

chadhietala requested changes Nov 8, 2019

View reviewed changes

rwjblue and others added 3 commits November 8, 2019 15:04

Fix a number of errors that occur when rehydrating malformed content.

39fecd6

Co-authored-by: Kris Selden <kselden@linkedin.com>

Add failing test for clearMismatch leaving nextSibling in invalid state

803e1d1

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from 76442d9 to 9a5d2c0 Compare November 8, 2019 20:18

krisselden previously requested changes Nov 9, 2019

View reviewed changes

packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts Outdated Show resolved Hide resolved

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from 9a5d2c0 to f32876c Compare November 12, 2019 20:39

rwjblue requested review from krisselden and chadhietala November 12, 2019 20:39

rwjblue and others added 4 commits November 20, 2019 09:23

Add tests for usage of Handlebars comments between text nodes.

4b4b5a6

Add failing test for openBlockDepth usage

a874927

Co-authored-by: Kris Selden <kselden@linkedin.com>

Add tests ensuring attributes are not mutated if their values match.

31680b6

rwjblue force-pushed the failing-test-for-closing-blocks-during-auto-correct-mismatch branch from 47b06ab to 31680b6 Compare November 20, 2019 14:58

rwjblue merged commit 2abbe46 into glimmerjs:master Nov 22, 2019

rwjblue deleted the failing-test-for-closing-blocks-during-auto-correct-mismatch branch November 22, 2019 15:35

rwjblue mentioned this pull request Dec 20, 2019

Rehydration: Cannot find serialized cursor for in-element emberjs/ember.js#18020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor RehydrateBuilder to be more robust #988

Refactor RehydrateBuilder to be more robust #988

rwjblue commented Nov 5, 2019 •

edited

Loading

rwjblue commented Nov 6, 2019

scalvert commented Nov 6, 2019

rwjblue Nov 8, 2019

rwjblue Nov 8, 2019

chadhietala left a comment

chadhietala Nov 8, 2019

rwjblue Nov 8, 2019

chadhietala Nov 8, 2019

rwjblue Nov 8, 2019

kratiahuja commented Nov 8, 2019

Refactor RehydrateBuilder to be more robust #988

Refactor RehydrateBuilder to be more robust #988

Conversation

rwjblue commented Nov 5, 2019 • edited Loading

rwjblue commented Nov 6, 2019

scalvert commented Nov 6, 2019

rwjblue Nov 8, 2019

Choose a reason for hiding this comment

rwjblue Nov 8, 2019

Choose a reason for hiding this comment

chadhietala left a comment

Choose a reason for hiding this comment

chadhietala Nov 8, 2019

Choose a reason for hiding this comment

rwjblue Nov 8, 2019

Choose a reason for hiding this comment

chadhietala Nov 8, 2019

Choose a reason for hiding this comment

rwjblue Nov 8, 2019

Choose a reason for hiding this comment

kratiahuja commented Nov 8, 2019

rwjblue commented Nov 5, 2019 •

edited

Loading