Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Launch Custom Elements v1 Polyfill #25141

Merged
merged 10 commits into from
Oct 19, 2019

Conversation

jridgewell
Copy link
Contributor

There don't seem to be any errors. Let's launch!

Note, we're still always polyfilling. But this gets us one step closer to using native Custom Elements v1 when supported (either via ES6 classes, or a minimal wrapper polyfill instead of a full polyfill).

There don't seem to be any errors. Let's launch!
@amp-owners-bot
Copy link

Hey @rsimha, these files were changed:

  • build-system/compile/compile.js

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Looks great Justin, nice work!

For future reviewers reference, this puts us on the path to removing the polyfill for modern browsers too!

@jridgewell
Copy link
Contributor Author

  • -4.55KB brotli
  • 6.3% smaller v0.js
  • 71.79 vs 67.24kb

@lannka
Copy link
Contributor

lannka commented Oct 18, 2019

This is great.

One question: are we utilizing the browser's native CustomElement support (with module-no-module)?

@jridgewell
Copy link
Contributor Author

No, we still always polyfill. We weren't confident that our use of Custom Elements matched the spec, so we decided to always polyfill. We can now begin experiments with using native CEv1.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Very nice! build-system/ changes LGTM.

@lannka
Copy link
Contributor

lannka commented Oct 18, 2019

/cc @keithwrightbos @jeffkaufman FYI, once merged it will be in RC next Tuesday. Please keep an eye on ads dashboard.

IE11 doesn't have `"[native code]"`, instead it returns `"[object HTMLElement]"`. Well,  if there's no CE, then we obviously need to polyfill anyways.
IE11 puts the `innerHTML` accessor on `HTMLElement.prototype`, not `Element.prototype`. And we've already replaced `HTMLElement` with our polyfill class. That polyfill class directly inherits from the native `HTMLElement`, so we can access it via the `__proto__` field.
@jridgewell jridgewell merged commit ea38284 into ampproject:master Oct 19, 2019
@jridgewell jridgewell deleted the custom-elements-v1-polyfill branch October 19, 2019 04:27
jeffjose pushed a commit to jeffjose/amphtml that referenced this pull request Oct 19, 2019
* Launch Custom Elements v1 Polyfill

There don't seem to be any errors. Let's launch!

* Always use customElements registry

* Fix isPatched check

IE11 doesn't have `"[native code]"`, instead it returns `"[object HTMLElement]"`. Well,  if there's no CE, then we obviously need to polyfill anyways.

* Fix IE11 innerHTML overwrite

IE11 puts the `innerHTML` accessor on `HTMLElement.prototype`, not `Element.prototype`. And we've already replaced `HTMLElement` with our polyfill class. That polyfill class directly inherits from the native `HTMLElement`, so we can access it via the `__proto__` field.
joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
* Launch Custom Elements v1 Polyfill

There don't seem to be any errors. Let's launch!

* Always use customElements registry

* Fix isPatched check

IE11 doesn't have `"[native code]"`, instead it returns `"[object HTMLElement]"`. Well,  if there's no CE, then we obviously need to polyfill anyways.

* Fix IE11 innerHTML overwrite

IE11 puts the `innerHTML` accessor on `HTMLElement.prototype`, not `Element.prototype`. And we've already replaced `HTMLElement` with our polyfill class. That polyfill class directly inherits from the native `HTMLElement`, so we can access it via the `__proto__` field.
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* Launch Custom Elements v1 Polyfill

There don't seem to be any errors. Let's launch!

* Always use customElements registry

* Fix isPatched check

IE11 doesn't have `"[native code]"`, instead it returns `"[object HTMLElement]"`. Well,  if there's no CE, then we obviously need to polyfill anyways.

* Fix IE11 innerHTML overwrite

IE11 puts the `innerHTML` accessor on `HTMLElement.prototype`, not `Element.prototype`. And we've already replaced `HTMLElement` with our polyfill class. That polyfill class directly inherits from the native `HTMLElement`, so we can access it via the `__proto__` field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants