Skip to content

✨ Launch minimal-wrapper native CEv1#26360

Merged
jridgewell merged 3 commits intomasterfrom
native-ce
Jan 23, 2020
Merged

✨ Launch minimal-wrapper native CEv1#26360
jridgewell merged 3 commits intomasterfrom
native-ce

Conversation

@jridgewell
Copy link
Copy Markdown
Contributor

@jridgewell jridgewell commented Jan 15, 2020

This pushes us closer to use real, native CEv1!

Now, instead of forcing a full polyfill in every browser, we will have the following support matrix:

CEv1 supported by browser? Classes are transpiled? Polyfill level
false false Full polyfill
false true Full polyfill
true false Minimal wrapper around HTMLElement
true true No polyfill

Note that we won't be able to eliminate the polyfill from module builds, since Firefox supported modules before CEv1. 😢

@kristoferbaxter
Copy link
Copy Markdown
Contributor

Delta in bundle size expected?

@jridgewell
Copy link
Copy Markdown
Contributor Author

Yes, it's expected. We're now including the minimal polyfill and the checks necessary to find which polyfill to install.

@jridgewell
Copy link
Copy Markdown
Contributor Author

Ping @cramforce and @choumx

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM with one question

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this the right type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure, but closure doesn't like the Function.call.call() below without it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add a comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

What were the observed results of the "Native Custom Elements V1" experiment?

* @param {!Function} ctor
*/
export function install(win, opt_ctor) {
export function install(win, ctor) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not just remove ctor parameter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because we still need to detect if we're in ESM (real classes) or ES5 (transpiled classes).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can be inlined in install() which would make the API a bit cleaner (optional).

win,
NATIVE_CUSTOM_ELEMENTS_V1 ? class {} : undefined
);
installCustomElements(win, class {});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We always transpile our testing code right? Did we try running our test suite against the native CE code path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if the ESM builds ran with this RTV experiment enabled. @erwinmombay?

@jridgewell
Copy link
Copy Markdown
Contributor Author

jridgewell commented Jan 22, 2020

We observed no latency impact at p95 and p50 for first contentful paint (or any metric), and no new unusual errors.

@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 23, 2020 via email

@jridgewell
Copy link
Copy Markdown
Contributor Author

No, everything was completely stable.

@jridgewell jridgewell merged commit 553061d into master Jan 23, 2020
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Jan 27, 2020
* master: (62 commits)
  📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491)
  Revert 'Move mutator implementations out to a standalone service' (ampproject#26479)
  Fix syntax error (ampproject#26481)
  Add pespective back. (ampproject#26486)
  More user friendly errors in layout.js (ampproject#26448)
  ✨ Start logging AMP URL on SwG Pages (ampproject#26480)
  Fix border around desktop amp-story-pages. (ampproject#26449)
  Fix Story tests. (ampproject#26464)
  ✨ Performance Measurement Chrome Extension (ampproject#26333)
  amp-consent restrict iframe fullScreen if no focus  (ampproject#26461)
  Add performance benchmark task (ampproject#26026)
  ♻️ amp-script: emit warning if zero height and width. (ampproject#26444)
  ✨ Launch minimal-wrapper native CEv1 (ampproject#26360)
  ♻️ Lint: include externs (round 2) (ampproject#26446)
  amp-script: Create 'fill content' container for responsive/fluid (ampproject#26400)
  amp-consent remove cmp iframe focus (ampproject#26437)
  Disable macro-after-long-task in inabox. (ampproject#26459)
  Launch layoutbox-invalidate-on-scroll (ampproject#26430)
  Add amp-ad support for ByPlay (ampproject#25663)
  🏗 Add specific RTV opt-in to experiments.html (ampproject#26434)
  ...
@rsimha rsimha deleted the native-ce branch February 13, 2020 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants