Add support for ShadyDOM on-demand patching #5585

sorvell · 2019-08-27T01:28:55Z

Do not merge until on webcomponents/polyfills#189.

…-onDemand

This is a polyfill optimization which defers class generation until first instance.

…fine` when the `Polymer` function is used.

kevinpschaaf

Approved, only a couple minor nits which I think are nbd.

kevinpschaaf · 2019-09-05T04:29:55Z

lib/elements/dom-repeat.js

@@ -347,11 +347,11 @@ export class DomRepeat extends domRepeatBase {
          /** @type {!HTMLElement} */ (this));
      let template = this.template = thisAsTemplate._templateInfo ?
          thisAsTemplate :
-          /** @type {!HTMLTemplateElement} */ (this.querySelector('template'));
+          /** @type {!HTMLTemplateElement} */ (wrap(this).querySelector('template'));


nbd, but nothing should ever be in a dom-repeat or dom-if except for a template (i.e. no nested elements with SR's that would need to be filtered out), so this is basically fine to always be native qs

kevinpschaaf · 2019-09-05T04:47:23Z

lib/utils/templatize.js

        } else {
          const replace = n.__polymerReplaced__;
          if (replace) {
-            wrap(wrap(replace).parentNode).replaceChild(n, replace);
+            wrap(wrapIfNeeded(replace).parentNode).replaceChild(n, replace);


Not totally clear why the outer wrap can't be wrapIfNeeded. Adding a <slot> to any random spot in a shadow root obviously wouldn't be automatically on-demand patched, but this is only putting a <slot> back into a node that already had it (which I think means it is guaranteed to be patched)?

kevinpschaaf · 2019-09-05T04:58:38Z

lib/mixins/property-effects.js

@@ -926,7 +926,7 @@ function setupCompoundStorage(node, binding) {
      // We may also want to consider doing this for `textContent` and
      // `innerHTML`.
      if (target === 'className') {
-        node = wrap(node);
+        node = wrapIfNeeded(node);


Seems like setting className on an otherwise uninteresting element in a SR (i.e. didn't need to have inside/outside patched) needs to always be wrapped so we don't lose scoping (note _setUnmanagedPropertyToNode below has similar code).

Might explain the test failure here? https://travis-ci.org/Polymer/polymer/builds/580988656#L780 (although unclear why this would only fail on Chrome 41?)

Fixed. The issue is that patch needs to be the fallback to wrapping so that this works when instance patching is required.

kevinpschaaf · 2019-09-05T16:26:13Z

lib/utils/wrap.js

@@ -22,3 +22,7 @@ export const wrap = (window['ShadyDOM'] && window['ShadyDOM']['noPatch'] && wind
  window['ShadyDOM']['wrap'] :
  (window['ShadyDOM'] ? (n) => ShadyDOM['patch'](n) : (n) => n);

+// Wrapping a local operation (e.g. appendChild) is only needed iff noPatch is
+// explicitly true, and on-demand patching is not used.
+export const wrapIfNeeded = (window['ShadyDOM'] && window['ShadyDOM']['noPatch'] === true && window['ShadyDOM']['wrap']) ?


Let's add comment to list known cases where wrap and not wrapIfNeeded is required:

When moving (appendChild/insertBefore) a node without an explicit removeChild to an unpatched location: in this case it needs to be unlinked from logical tree, have its className unscoped, trigger slotchange and, trigger distribution if it was a <slot>

When setting className to an unpatched node: if it was in a SR it needs to go through ShadyCSS to avoid losing its scoping class

When calling querySelector on an unpatched node: it won't return scoped results

When getting textContent or innerHTML on an unpatched node: it won't return scoped results

Also, now wondering if wrap and wrapAlways might be better naming (and less annoying to boot)?

* Fixes an issue when instance patching is required * Now uses ShadyDOM.wrapIfNeeded.

…ntMixn are pre-patched.

…-onDemand

kevinpschaaf · 2019-10-11T00:44:58Z

lib/utils/wrap.js

+ * when setting `textContent` or `innerHTML`.
+ */
+export const wrapIfNeeded = window['ShadyDOM'] && window['ShadyDOM']['wrapIfNeeded'] ?
+  window['ShadyDOM']['wrapIfNeeded'] : n => n;


fall back to wrap?

kevinpschaaf · 2019-10-11T00:49:44Z

test/unit/lazy-define.html

+
+    test('Polymer function does not return class when lazy define is used', function() {
+      assert.notOk(window.XLazyDeclarative);
+      assert.notOk(window.XLazyImperative);


window.XLazyImperative was never assigned above

kevinpschaaf · 2019-10-11T01:14:06Z

test/unit/lazy-define.html

+      assert.ok(e.root.querySelector('span'));
+    });
+
+    test('imperatively generated elements upgrade and customElements.get works after instance created', function() {


Add a test for something like

Polymer.telemetry.registrations.filter(r => r.is == 'x-lazy-imperative').length === 0

?

…-onDemand

stale · 2020-10-14T01:55:44Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Add support for ShadyDOM on-demand patching

fa293fa

sorvell requested a review from kevinpschaaf August 27, 2019 01:28

googlebot added the cla: yes label Aug 27, 2019

Steven Orvell added 4 commits August 29, 2019 14:17

Merge branch 'legacy-undefined-noBatch' into legacy-undefined-noBatch…

89d1f28

…-onDemand

Adds support for customElements.polyfillDefineLazy

0699a43

This is a polyfill optimization which defers class generation until first instance.

Adds lazyDefine setting for enabling `customElements.polyfillLazyDe…

2498031

…fine` when the `Polymer` function is used.

lint fixes

078cb34

kevinpschaaf changed the base branch from master to legacy-undefined-noBatch September 4, 2019 23:58

Steven Orvell added 4 commits September 4, 2019 17:05

Change based on review: rename wrapLocal to wrapIfNeeded

e0c4e82

Use wrapIfNeeded where possible.

31c810e

More wrapIfNeeded.

a7f9c0f

Use wrap in dom-repeat.

454cb54

kevinpschaaf approved these changes Sep 5, 2019

View reviewed changes

kevinpschaaf requested changes Sep 5, 2019

View reviewed changes

kevinpschaaf reviewed Sep 5, 2019

View reviewed changes

Steven Orvell added 11 commits September 5, 2019 11:41

Refinements to wrap

4422d13

* Fixes an issue when instance patching is required * Now uses ShadyDOM.wrapIfNeeded.

Lint fixes and added wrapping comment.

19abf83

Add noPatch == on-demand tests

39f9853

ShadyDOM on-demand optimization: ensure all elements that mixin Eleme…

d545c8f

…ntMixn are pre-patched.

Add comment about wrapping events.

55aa8c8

getRootNode must be wrapped

3bc3cc3

Merge branch 'legacy-undefined-noBatch' into legacy-undefined-noBatch…

7164daf

…-onDemand

Run all tests with ShadyDOM on-demand setting.

107dcc7

Add tests for lazyDefine setting

d0acc28

wrapIfNeeded fall back to wrap

13186e7

Use wrap instead of ShadyDOM.wrap

94eea4a

kevinpschaaf requested changes Oct 11, 2019

View reviewed changes

Steven Orvell and others added 3 commits October 10, 2019 18:30

Adds telemetry test

e4c7379

Merge branch 'legacy-undefined-noBatch' into legacy-undefined-noBatch…

105d203

…-onDemand

Add lazyDefine setting, remove obsolete settings.

cd6793a

stale bot added the wontfix label Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ShadyDOM on-demand patching #5585

Add support for ShadyDOM on-demand patching #5585

sorvell commented Aug 27, 2019

kevinpschaaf left a comment

kevinpschaaf Sep 5, 2019

sorvell Sep 5, 2019

kevinpschaaf Sep 5, 2019

sorvell Sep 5, 2019

kevinpschaaf Sep 5, 2019 •

edited

Loading

sorvell Sep 5, 2019

kevinpschaaf Sep 5, 2019

sorvell Sep 5, 2019

kevinpschaaf Oct 11, 2019

kevinpschaaf Oct 11, 2019

kevinpschaaf Oct 11, 2019

stale bot commented Oct 14, 2020

Add support for ShadyDOM on-demand patching #5585

Are you sure you want to change the base?

Add support for ShadyDOM on-demand patching #5585

Conversation

sorvell commented Aug 27, 2019

kevinpschaaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinpschaaf Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Oct 14, 2020

kevinpschaaf Sep 5, 2019 •

edited

Loading