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 minimal-wrapper native CEv1 #26360

Merged
merged 3 commits into from
Jan 23, 2020
Merged

✨ Launch minimal-wrapper native CEv1 #26360

merged 3 commits into from
Jan 23, 2020

Conversation

jridgewell
Copy link
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
Contributor

Delta in bundle size expected?

@jridgewell
Copy link
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
Contributor Author

Ping @cramforce and @choumx

Copy link
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

@@ -958,7 +958,7 @@ export function install(win, ctor) {
const {Reflect} = win;

// "Construct" ctor using ES5 idioms
const instance = Object.create(ctor.prototype);
const instance = /** @type {!Function} */ (Object.create(ctor.prototype));
Copy link
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
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
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
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

@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?

*/
export function install(win, opt_ctor) {
export function install(win, ctor) {

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
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).

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 {});

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
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
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
Member

cramforce commented Jan 23, 2020 via email

@jridgewell
Copy link
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