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

Use Custom Elements v1 API #67

Merged
merged 12 commits into from
Jan 3, 2018
Merged

Use Custom Elements v1 API #67

merged 12 commits into from
Jan 3, 2018

Conversation

josh
Copy link
Contributor

@josh josh commented Nov 22, 2016

Transitioning to Custom Elements v1.

  • Figure out Reflect.construct polyfill situation. @notwaldorf how is polymer implementing Custom Elements v1 on old browsers?
  • Fix weirdness using web-components.js polyfill on Safari TP.
  • Consider refactoring as ES Modules. import {LocalTimeElement} from 'time-elements' is side-effect free. Also support import 'time-elements/define' which registers the constructors on the current document.

To: @dgraham @mislav
CC: @github/js @notwaldorf

@tuespetre tuespetre mentioned this pull request Nov 22, 2016
@justinfagnani
Copy link

Hey @josh! @notwaldorf asked me to comment on the Reflect.construct situation.

I would stay far, far away from Reflect.construct, as it doesn't actually serve as a replacement for super() and is very slow.

The basic problem is that V1 Custom Elements essentially must be written as ES6 classes. This is because it's not possible for an ES5-style class to extend an ES6 class because it can't invoke super() - the ES5 pattern of MySuperClass.call(this) is invoking a constructor as a function. All extendible built-ins like Array, Map and HTMLElement are effectively ES6 classes and require a super() call into their constructor. Reflect.construct isn't a way to invoke super(), it's a way to invoke new C() which allocates a new instance, so code like this:

function RelativeTimeElement() {
  var self = Reflect.construct(HTMLElement, [], RelativeTimeElement);
  return self;
}
new RelativeTimeElement();

is creating 2 objects: one at the new RelativeTimeElement() call, which is thrown away, and another at the Reflect.construct call. The situation gets worse if you have a deep class hierarchy as you generate one garbage object and make one slow Reflect.construct call per level in the hierarchy. It looks like you have a hierarchy here too.

Since there is no way to perform a super() call in ES5, our recommendation is to write and distribute all Custom Elements as ES6 classes, and compile to ES5 only for those browsers that don't support ES6. Since they will be using a polyfill anyway, there's no problem with compiling all custom elements class hierarchies to ES5. If you do need to deliver the same assets to all browsers, you can compile the whole app to ES5, and use the Custom Elements polyfill's "native shim" which patches up HTMLElement and window.customElement to be able to accept ES5-style classes. This requires that all custom elements be compiled to ES5 though, so it's another reason why we recommend writing everything in ES6 and just compiling the whole app.

The native-shim is here, along with some details about why it's needed: https://github.com/webcomponents/custom-elements/blob/master/src/native-shim.js

I hope that helps!

@josh
Copy link
Contributor Author

josh commented Nov 22, 2016

Shipping an ES6 class seems like the right way to go for this repository.

Though, "compile to ES5 only for those browsers that don't support ES6" seems to be glossed over. Has someone implemented a Babel transform that can correct extend HTMLElement? Or does the existing code work as long as its paired with the custom elements polyfill?

Thanks!

@justinfagnani
Copy link

Babel compiled classes work as long as either the polyfill or the native-shim are loaded (this is because constructors in Babel output correctly return the value of the "super" call, the self variable in this PR). TypeScript and Closure have recently been updated to do this too.

@josh
Copy link
Contributor Author

josh commented Nov 23, 2016

Reflect.construct seems to be a pretty good approach for ES5 compat. The main thing is that it works as is with ES5 compiled classes on Chrome and Safari without any additional runtime support. Your Custom Elements polyfill seems to do the right thing in regardless to Reflect.construct.

In terms of publishing, I'm proposing this package ship two source files.

  • Future looking ES6 syntax extending HTMLElement. Works as is if browser supports Custom Elements V1. Serves as an option for those wanting to transpile via an alternative method.
  • ES5 compiled class using Reflect.construct transform. This file works in all modern browsers as well as legacy ones. I just wrote this babel-plugin-transform-custom-element-classes transform that can automate the compilation from the ES6 source here to this as distribution target.

@tuespetre
Copy link

@josh I have experimented with delivering ES2015 classes to browsers that support it, and in those that do, like Edge, the call to super() seemed to work well with shimmed HTMLElement constructors. But the browsers that don't support ES2015 class syntax also tend to not support Reflect.construct, and if the constructor has to be shimmed anyways, an ES5 class might as well just .call into it.

@mislav
Copy link
Contributor

mislav commented Nov 24, 2016

I have experimented with delivering ES2015 classes to browsers that support it

@tuespetre How do you determine whether a browser supports ES2015 before you deliver JS files to it?

@tuespetre
Copy link

@mislav I use an approach like @philipwalton describes in his blog post:

(function () {

    // ...some other scripts and checks

    if (es2015SyntaxSupported()) {
        addScript('/path/to/some-component.es2015.js');
    }
    else {
        addScript('/path/to/some-component.es5.js');
    }

    // ...some other scripts and checks

    function addScript(src) {
        var script = document.createElement('script');
        script.src = src;
        script.async = false;
        document.head.appendChild(script);
    }

    function es2015SyntaxSupported() {
        try {
            // Cram as much ES2015 syntax as possible into here
            return new Function('for(const{_}of[((..._)=>class{constructor(){new.target}})(...[``])]){let $={_}}');
        }
        catch (error) {
            return false;
        }
    }

})();

@mislav
Copy link
Contributor

mislav commented Nov 24, 2016

@tuespetre I see; thanks for showing! 🙇

@tuespetre
Copy link

@josh I see in the changes that window.WhateverElement is still being set; maybe this would be a good time to stop setting a global variable and instead direct users towards window.customElements.get('local-time') (or other) instead.

@josh
Copy link
Contributor Author

josh commented Nov 29, 2016

maybe this would be a good time to stop setting a global variable and instead direct users towards window.customElements.get('local-time') (or other) instead.

I don't really see the practical difference. Both are global registrations. The current implementation of the Custom Elements Registry doesn't provide any scoping mechanize.

@tuespetre
Copy link

@josh the primary benefit of using the CustomElementRegistry instead would be the ability to use whenDefined.

Any plans to move forward with this PR? 😸

@muan muan mentioned this pull request Dec 19, 2017
@muan muan mentioned this pull request Dec 20, 2017
Copy link
Contributor Author

@josh josh left a comment

Choose a reason for hiding this comment

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

@muan your changes look good here. Thanks! If you feel like we should iterate on V1 in master, we can go ahead and merge this branch.

@muan muan merged commit 90a1745 into master Jan 3, 2018
@muan muan deleted the custom-elements-v1 branch January 3, 2018 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants