cramforce / splittable

Module bundler with support for code splitting, ES6 & CommonJS modules.
Apache License 2.0
945 stars 34 forks source link

Incorrectly transpiles classes #8

Closed matthewp closed 7 years ago

matthewp commented 7 years ago

This input code:

class MyWidget extends HTMLElement {
  connectedCallback(){
    console.log("hello world");
  }
}

window['customElements']['define']('my-widget', MyWidget);

Compiles to:

function f(a, b) {
    function d() {}
    d.prototype = b.prototype;
    a.prototype = new d;
    a.prototype.constructor = a;
    for (var c in b)
        if (Object.defineProperties) {
            var e = Object.getOwnPropertyDescriptor(b, c);
            e && Object.defineProperty(a, c, e)
        } else
            a[c] = b[c]
}
function g(a) {
    HTMLElement.apply(this, arguments)
}
f(g, HTMLElement);
window.customElements.define("my-widget", g);

Note the HTMLElement.apply; you can't do that. It should instead transpile to: return Reflect.construct(HTMLElement, arguments, this.constructor). I think this still wouldn't work without a shim though.

Ideally there would be a way to turn off -> ES5 transpiling though, as it is more verbose than the ES6 equivalent. Also, in this case by using Reflect.construct you would be doubling the number of objects that get created. The ES class version is just better.

Maybe I should post an issue in Closure's issue tracker for this though?

cramforce commented 7 years ago

Yeah, nothing splittable can do about it. Closure certainly favors fast over correct, although that transpile is pretty suspicious for various reasons.

On Nov 17, 2016 9:37 AM, "Matthew Phillips" notifications@github.com wrote:

This input code:

class MyWidget extends HTMLElement { connectedCallback(){ console.log("hello world"); } } window['customElements']['define']('my-widget', MyWidget);

Compiles to:

function f(a, b) { function d() {} d.prototype = b.prototype; a.prototype = new d; a.prototype.constructor = a; for (var c in b) if (Object.defineProperties) { var e = Object.getOwnPropertyDescriptor(b, c); e && Object.defineProperty(a, c, e) } else a[c] = b[c] }function g(a) { HTMLElement.apply(this, arguments) }f(g, HTMLElement);window.customElements.define("my-widget", g);

Note the HTMLElement.apply; you can't do that. It should instead transpile to: return Reflect.construct(HTMLElement, arguments, this.constructor). I think this still wouldn't work without a shim though.

Ideally there would be a way to turn off -> ES5 transpiling though, as it is more verbose than the ES6 equivalent. Also, in this case by using Reflect.construct you would be doubling the number of objects that get created. The ES class version is just better.

Maybe I should post an issue in Closure's issue tracker for this though?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cramforce/splittable/issues/8, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT8qp7c7P7Po-63VghWvvUUCprBKHks5q_JDcgaJpZM4K1mDs .

matthewp commented 7 years ago

I'm not sure why this code would be faster than the correct ES2015 version though. Even if you replaced the HTMLElement.apply (which throws currently) with a Reflect.construct call it would be slower.

cramforce commented 7 years ago

Lets move to Closure Compiler github https://github.com/google/closure-compiler

matthewp commented 7 years ago

I posted https://github.com/google/closure-compiler/issues/2157, thanks!