🚀♻️ Custom Elements V1 Polyfill #17205

jridgewell · 2018-07-30T23:40:15Z

Uses a custom baked Custom Elements V1 polyfill, supporting just the parts we need (connectedCallback, disconnectedCallback, whenDefined, define) using just the features available (MutaitonObserver) in the browsers we target (recent Chrome, Safari 7+, recent Firefox, IE11+).

Preliminary size is 85% smaller than the (much more complete)document.registerElement polyfill (1.5kb vs 9.5kb).

This supports only "autonomous custom elements" from Custom Elements V1 (not v0). This does not polyfill extending native built-in elements (we don't do this in AMP). This limited V1 use case has been implemented since Chrome 54 and Safari 10.3.

This supports using the native Custom Elements V1, if available. Both ES5 style classes and ES6 native classes are supported (but not at the same time, it's one or the other). If ES6 native classes are used (as in the upcoming AMP module build), nothing is patched. If ES5 classes are used (as in regular AMP), only HTMLElement is patched.

If native Custom Elements V1 is not supported, this does a full patch. This includes document.createElement and friends, and uses a MutationObserver to detect when nodes become connected/disconnected. In this full-patch mode, Custom Elements are not supported in the shadowy part of ShadowRoots (they are supported in the slots of a shadow root, aka the regular DOM).

Oh, and there are no issues installing into child windows. So, fixes #16052.

I think this was a pretty good Fixit.

kristoferbaxter · 2018-07-30T23:42:07Z

Incredibly excited to review this. Thank you for the hard work!

jridgewell · 2018-07-30T23:46:57Z

build-system/tasks/compile.js

@@ -295,6 +295,8 @@ function compile(entryModuleFilenames, outputDir,
          '!build/fake-module/src/polyfills/**/*.js',
          '!build/fake-polyfills/src/polyfills.js',
          '!src/polyfills/*.js',
+          // TODO(prateekbh): I don't understand how to add
+          // src/polyfills/custom-elements.js to the _needed_ polyfills.


do we need to exclude src/polyfills/custom-elements.js for the module build?

cuz last time we checked with Malte, the lifecycle without custom elements polyfills is skewed.

No, we need to include it in all builds. I don’t know how to do that for the module build.

By default it will be added in all.
Extra work is for exclusion

Are you sure? The previous line ('!src/polyfills/*.js') excludes everything from polyfills directory.

Oh! with your PR we wont need to shadow it anywhere else, gotcha.

best way would be to remove the above line and add a code below this

polyfillsShadowList.forEach(polyfillFile => { srcs.push(`!src/polyfills/${polyfillFile}.js`); })

this will exclude all but your polyfill

cramforce · 2018-07-31T15:56:33Z

src/polyfills/custom-elements.js

+ *   resolve: function(),
+ * }}
+ */
+let DeferredDef;


This currently has no tests, does it?

None yet. I've manually tested, and was going to build this into a fully fledged node module with tests there.

Note that the our regular tests are going to run with this polyfill instead of document.registerElement. If that's not enough, I can begin the test suite in this repo and extract all of it out later?

Have you actually tried running the tests with the experiment on?

https://github.com/ampproject/amphtml/pull/17205/files#diff-8f8103e0f4e0052eab727bfd5c21e090R107

https://github.com/ampproject/amphtml/pull/17205/files#diff-1179de12a95783d46f8bf0056a156c74R28

I can change the experiment check to test for getMode().test too, if we want to ensure all tests run with it.

I think this is missing most integration tests that use a fully built AMP.

Made it so it always runs in tests.

But not yet in integration tests, right?

No, all test run with the new polyfill. At the moment, this includes:

SL_Chrome_67

SL_Firefox_61

SL_Safari_11

We can enable more saucelabs browsers, but I don't think that's absolutely necessary until we're ready to take this out of opt-in experiment?

cramforce · 2018-07-31T15:57:13Z

src/polyfills/custom-elements.js

+}
+
+/**
+ * Was HTMLElement already patched this window?


Nit: missing for

cramforce · 2018-07-31T15:58:12Z

src/polyfills/custom-elements.js

+ */
+function isPatched(win) {
+  const tag = win.HTMLElement.toString();
+  return tag.indexOf('[native code]') === -1;


Is this a good check? [native code] appears in bound fn.

Not full proof, but I don't think it's likely someone will obscure the toString of the HTMLElement constructor. I wanted to avoid doing a creation test using a subclass of the constructor, because it would require polluting the custom elements registry.

I can try thinking of a better solution if you're not comfortable with it.

Magic property on polyfilled class?

I was trying to allow PWA publishers to include a fully featured Custom Elements for their own code.

If they do, it's likely they'll patch HTMLElement as part of the polyfill. In that case, wrapping the HTMLElement constructor is unnecessary. But I can't do a full feature test to see if HTMLElement will work in our case unless I register my own CE and try document.createElement to see if it'll throw. This seemed like a good enough approach.

cramforce · 2018-07-31T15:58:35Z

src/polyfills/custom-elements.js

+    const lifecycleCallbacks = {
+      'connectedCallback': null,
+      'disconnectedCallback': null,
+      // 'adoptedCallback': null,


These aren't actually used in AMP, so I started comment them out. I can remove these and leave TODOs instead.

But we do use attribute change callback, right? And you implement it below?

I thought so, but Bind created its own mutatedAttributesCallback and there we no matches for attributeChangedCallback when grepping.

cramforce · 2018-07-31T15:59:39Z

src/polyfills/custom-elements.js

+     */
+    this.current_ = null;
+
+    const observer = new win.MutationObserver(records => {


Is this everywhere we care about?

Yup. IE11, Chrome 26, FF 14, Safari 7, even Samsung's "Internet".

cramforce · 2018-07-31T21:14:36Z

src/polyfills/custom-elements.js

+/**
+ * Polyfills Custom Elements v1 API
+ * @param {!Window} win
+ * @param {!Function} ctor


This badly needs docs

cramforce · 2018-07-31T21:16:06Z

src/polyfills/custom-elements.js

+ *   resolve: function(),
+ * }}
+ */
+let DeferredDef;


Have you actually tried running the tests with the experiment on?

jridgewell

Added copious comments.

jridgewell · 2018-07-31T23:05:46Z

src/polyfills/custom-elements.js

+ *   resolve: function(),
+ * }}
+ */
+let DeferredDef;


Made it so it always runs in tests.

cramforce · 2018-07-31T23:20:56Z

src/polyfills.js

  'document-register-element/build/document-register-element.patched';

-installCustomElements(self, 'auto');
+if (isExperimentOn(self, 'custom-elements-v1') || getMode().test) {


You want at least one test that ensures the old still works. That is unless you are super optimistic about moving over.

I'm more worried about bugs with using native custom elements than I am with this polyfill not matching the current polyfill. Worst case we move to this one, and disable the native CE/Reflect.construct support.

kristoferbaxter · 2018-08-01T18:29:52Z

build-system/tasks/bundle-size.js

@@ -22,7 +22,7 @@ const log = require('fancy-log');
 const {getStdout} = require('../exec');

 const runtimeFile = './dist/v0.js';
-const maxSize = '79.28KB';
+const maxSize = '80.60KB';


Did the maxSize increase because we are shipping both polyfills in this PR?

kristoferbaxter · 2018-08-01T19:09:55Z

src/polyfills/custom-elements.js

+ * This intentionally ignores "valid" higher Unicode Code Points.
+ * https://html.spec.whatwg.org/multipage/custom-elements.html#valid-custom-element-name
+ */
+const VALID_NAME = /^[a-z][a-z0-9._]*-[a-z0-9._-]*$/;


I'm curious if this should be different for AMP's specific usecase. Could we only allow names matching amp '-' [a-z] (PCENChar)*?

Anything else wouldn't be a valid AMP component.

This would eliminate the need for an invalid names list of strings.

I'd rather leave this generic since the plan is to extract this into a node module. We can add amp- prefix requirements at AMP's registerElement if we want.

cramforce · 2018-08-01T20:22:13Z

I'm not actually a fan of the amp- prefix on our custom elements. At least there might be scenarios in the future where not all supported elements have it.

…

On Wed, Aug 1, 2018 at 12:16 PM Kristofer Baxter ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/polyfills/custom-elements.js <#17205 (comment)>: > +let CustomElementConstructorDef; + +/** + * @typedef {{ + * name: string, + * ctor: !CustomElementConstructorDef, + * }} + */ +let CustomElementDef; + +/** + * Validates the custom element's name. + * This intentionally ignores "valid" higher Unicode Code Points. + * https://html.spec.whatwg.org/multipage/custom-elements.html#valid-custom-element-name + */ +const VALID_NAME = /^[a-z][a-z0-9._]*-[a-z0-9._-]*$/; This would eliminate the need for an invalid names list of strings. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#17205 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFeT8XxpQDHOsT4WMU9BYhKcUJzV-P5ks5uMf6MgaJpZM4VnW8X> .

jridgewell requested review from cramforce and dreamofabear July 30, 2018 23:40

googlebot added the cla: yes label Jul 30, 2018

jridgewell requested a review from kristoferbaxter July 30, 2018 23:40

jridgewell commented Jul 30, 2018

View reviewed changes

cramforce requested changes Jul 31, 2018

View reviewed changes

cramforce reviewed Jul 31, 2018

View reviewed changes

jridgewell commented Jul 31, 2018

View reviewed changes

src/polyfills/custom-elements.js

* resolve: function(),

* }}

*/

let DeferredDef;

Copy link

Contributor Author

jridgewell Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it so it always runs in tests.

cramforce reviewed Jul 31, 2018

View reviewed changes

kristoferbaxter reviewed Aug 1, 2018

View reviewed changes

jridgewell added 10 commits August 1, 2018 16:30

tmp

8b525a7

Fixes

94d836a

Wrap HTMLElement if Reflect.construct is available

f09eebf

Completely isolate code from globals

c6f0c7e

Install in child win

ad2c0eb

Fix

68cf92f

Remove document-register-element

01d7530

Rename file

70a70ff

Fix types

dc51611

Experimentally use new custom elements polyfill

f821d00

jridgewell mentioned this pull request Aug 1, 2018

Cleanup 'custom-elements-v1' experiment #17243

Closed

jridgewell added 6 commits August 1, 2018 16:31

Register custom-elements-v1 experiment

35ebc6d

Fix comment

b5072fc

Include custom elements polyfill with module build

16ac093

Fix build system

6b7a687

Bump build size

ba171aa

tmp

eaf7422

rsimha mentioned this pull request Aug 16, 2018

Avoid calling Object.assign before polyfill is installed #17544

Merged

jridgewell mentioned this pull request Sep 5, 2018

Initialization of custom elements in friendly iframe causes error to be logged. #16416

Closed

dhanendrannewscorp mentioned this pull request Sep 12, 2018

Error: "Cannot read property 'create' of undefined" #17997

Closed

ampprojectbot mentioned this pull request Apr 27, 2020

🚨 Error: Cannot read property 'set' of undefined #28058

Closed

ampprojectbot mentioned this pull request Mar 16, 2021

🚨 Error: Cannot read property 'toString' of undefined ampproject/error-reporting#70

Open

This was referenced Oct 19, 2021

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36406

Closed

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36407

Closed

🚨 Error: viewport has zero dimensions #36463

Closed

🚀♻️ Custom Elements V1 Polyfill #17205

🚀♻️ Custom Elements V1 Polyfill #17205

Conversation

jridgewell commented Jul 30, 2018 • edited Loading

kristoferbaxter commented Jul 30, 2018

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

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

jridgewell Aug 3, 2018 • 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

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

Choose a reason for hiding this comment

jridgewell 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

Choose a reason for hiding this comment

kristoferbaxter Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Aug 1, 2018 via email

jridgewell commented Jul 30, 2018 •

edited

Loading

jridgewell Aug 3, 2018 •

edited

Loading

kristoferbaxter Aug 1, 2018 •

edited

Loading