-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,5 @@ | ||
{ | ||
"experimentA": {}, | ||
"experimentB": {}, | ||
"experimentC": { | ||
"name": "Native Custom Elements V1", | ||
"environment": "AMP", | ||
"command": "gulp dist --defineExperimentConstant=NATIVE_CUSTOM_ELEMENTS_V1", | ||
"issue": "https://github.com/ampproject/amphtml/issues/17243", | ||
"expirationDateUTC": "2020-01-01", | ||
"defineExperimentConstant": "NATIVE_CUSTOM_ELEMENTS_V1" | ||
} | ||
"experimentC": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -937,9 +937,9 @@ export function copyProperties(obj, prototype) { | |
* done. | ||
* | ||
* @param {!Window} win | ||
* @param {!Function=} opt_ctor | ||
* @param {!Function} ctor | ||
*/ | ||
export function install(win, opt_ctor) { | ||
export function install(win, ctor) { | ||
// Don't install in no-DOM environments e.g. worker. | ||
const shouldInstall = win.document; | ||
const hasCE = hasCustomElements(win); | ||
|
@@ -950,23 +950,24 @@ export function install(win, opt_ctor) { | |
let install = true; | ||
let installWrapper = false; | ||
|
||
if (opt_ctor && hasCE) { | ||
if (ctor && hasCE) { | ||
// If ctor is constructable without new, it's a function. That means it was | ||
// compiled down, and we need to do the minimal polyfill because all you | ||
// cannot extend HTMLElement without native classes. | ||
try { | ||
const {Reflect} = win; | ||
|
||
// "Construct" ctor using ES5 idioms | ||
const instance = /** @type {!Function} */ (Object.create( | ||
opt_ctor.prototype | ||
)); | ||
// I'm not sure why, but Closure will complain at the | ||
// `Function.call.call()` below unless we cast to a Function instance | ||
// here. | ||
const instance = /** @type {!Function} */ (Object.create(ctor.prototype)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this the right type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really sure, but closure doesn't like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
// This will throw an error unless we're in a transpiled environemnt. | ||
// Native classes must be called as `new Ctor`, not `Ctor.call(instance)`. | ||
// We use `Function.call.call` because Closure is too smart for regular | ||
// `Ctor.call`. | ||
Function.call.call(opt_ctor, instance); | ||
Function.call.call(ctor, instance); | ||
|
||
// If that didn't throw, we're transpiled. | ||
// Let's find out if we can wrap HTMLElement and avoid a full patch. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -648,10 +648,7 @@ class RealWinFixture { | |
} else { | ||
// The anonymous class parameter allows us to detect native classes | ||
// vs transpiled classes. | ||
installCustomElements( | ||
win, | ||
NATIVE_CUSTOM_ELEMENTS_V1 ? class {} : undefined | ||
); | ||
installCustomElements(win, class {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
|
||
// Intercept event listeners | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).