WebReflection / babel-plugin-transform-builtin-classes

A fix for the infamous Babel #4480 bug.
https://github.com/babel/babel/issues/4480
ISC License
43 stars 2 forks source link

I'm using class factory mixins. #6

Closed trusktr closed 7 years ago

trusktr commented 7 years ago
export default
function WebComponent(base) {
  base = base || HTMLElement
  return class WebComponent extends base { ... }
}

A user might pass anything in: HTMLImageElement, HTMLButtonElement, or some class extending from a native one, etc.

Will this still work?

trusktr commented 7 years ago

I haven't tried yet or looked at it yet, but since I'm looking to support Edge 12+ which has classes and Proxy, would classtrophobic classes passed into WebComponent work?

WebReflection commented 7 years ago

Will this still work?

whenever that base is defined, if it's extending HTMLElement it will be fixed.

If it's an external dependency that doesn't pass through babel then I assume they know how to extend so nothing to worry about.

About classtrophobic, it's designed to avoid Babel so whatever happens with usual code is guarded via classtrophobic. My tests with custom-tag showed it works with the poly and all browsers you are targeting, because classtrophobic use native classes right away or it use the proper way to extend (the ES5 version).

However, classtrophobic was born before this repo, today I might not suggest its usage anymore 'cause it's a diversion to standards.

trusktr commented 7 years ago

In my code, eventually that base is defined like this f.e.:

import WebComponent from './WebComponent'
class CoolElement extends WebComponent() { // HTMLElement by default
  //  ...
}

Is transform-builtin-classes supposed to statically analyze this to see that base will be HTMLElement?

I've this config:

                ['transform-builtin-classes', {
                    globals: ['HTMLElement']
                }],

The Babel output is basically this:

function WebComponent(elementClass) {
    if (!elementClass) {
        elementClass = HTMLElement;
    }

    let WebComponent = function (_elementClass) {
        inherits(WebComponent, _elementClass);

        function WebComponent() {
            classCallCheck(this, WebComponent);

            var _this = possibleConstructorReturn(this, (WebComponent.__proto__ || Object.getPrototypeOf(WebComponent)).call(this));

I don't see it applying any fix.

trusktr commented 7 years ago

Took a look at the source, fixBabelExtend is not anywhere in my bundle (rolluped using rollup-plugin-babel with that globals config).

trusktr commented 7 years ago

I'm trying to compile classes with transform-builtin-classes because I'm just trying different solutions to see what works. I'd like to write classes (obviously) and have them work with CE v1 polyfilled (f.e. Edge) or not (f.e. Chrome).

Ideally I don't want to transpile classes, and just get native classes working with CE v1.

I'm aiming for end users of my element classes to be able to do the following in their applications:

import SomethingJoeMade from 'something-joe-made'

// using polyfill or not:
customElements.define('any-name-user-wants', SomethingJoeMade)

or

var {SomethingJoeMade} = joeLib // from global
customElements.define('any-name-user-wants', SomethingJoeMade)

Is there a best approach that you'd recommend in order not to have to compile classes (or compiling classes if no other option), and for users to be able to define elements in their own way with CE v1?

WebReflection commented 7 years ago

Dunno, I find it a bit odd you want third parts to define the element name for your classes.

It makes sense to avoid namespaces/tag-names conflicts, but I've never thought about it.

The problem is that today every Web developer trapped herself behind tools they don't control and more than fix most common cases, I wouldn't know how to avoid all the possible issues in every possible build-toolchain hell, sorry.

trusktr commented 7 years ago

It makes sense to avoid namespaces/tag-names conflicts, but I've never thought about it.

Exactly, I'd rather let them have the option to either useDefaultNames() if they want something easy, or be able to define their own element names for my classes manually.

Do you have a project that you could link me to, that uses CE v1 in IE or Edge, so I can see how it works?

trusktr commented 7 years ago

This only way I can get fixBabelExtend to appear anywhere in the output bundle is to (1) remove transform-es2015-classes, so my config looks like this:

            plugins: [
                //'transform-es2015-classes',
                ['transform-builtin-classes', {
                    globals: ['HTMLElement']
                }],
                'external-helpers',
            ],

and then (2) instead of using a WebComponent(base) mixin, I have to change it to

class WebComponent extends HTMLElement { ... }

rather than something like

function WebComponent(base) {
  base = base || HTMLElement
  class WebComponent extends base { ... }
}

and then the output looks like this:

// underscore prefixed by Rollup
_fixBabelExtend(class WebComponent extends HTMLElement)

If I revert either of those two changes, then fixBabelExtend does not appear in the output bundle. It seems like this doesn't work with mixins.

trusktr commented 7 years ago

Aha, I can also make fixBabelExtend appear in the output ("builtin extends patched") if I

  1. change the order of the plugin in rollup config:

            plugins: [
                ['transform-builtin-classes', {
                    globals: ['HTMLElement']
                }],
                'transform-es2015-classes',
                'external-helpers',
            ],

    and

  2. Stop using a mixin (class WebComponent extends HTMLElement {} directly)

Soon as I change back to a mixin, builtin extends patched stops happening.

trusktr commented 7 years ago

Cool, at least now, with direct extends HTMLElement rather than mixin, I can verify that the transpiled ES5 code works with native CE v1 in Chrome!

How can we get this working with mixins?

WebReflection commented 7 years ago

Do you have a project that you could link me to, that uses CE v1 in IE or Edge, so I can see how it works?

Google AMP pages use this polyfill: https://www.ampproject.org StencilJS uses this polyfill: https://stenciljs.com AFrame uses this polyfill: https://aframe.io

I believe these all work in IE or Edge too.

I can also make fixBabelExtend appear in the output ("builtin extends patched") if I change the order of the plugin in rollup config

Funny in Babel that goes exactly in reverted order, as mentioned in the README: https://github.com/WebReflection/babel-plugin-transform-builtin-classes#usage

If this is confirmed I think I should update the README with Rollup specific hint.

How can we get this working with mixins?

maybe making the intent statically analyzable?

function WebComponent(base) {
  if (!base) base = class extends HTMLElement;
  class WebComponent extends base { ... }
}

that should wrap base when not available, while when available it should be already done upfront (I mean the inheriting HTMLElement and the babel patch circle).

Would that work?

trusktr commented 7 years ago

Just notes:

AFrame uses this polyfill: https://aframe.io

They use the v0 API for now, holding off on moving to v1 because of current issues in general.

StencilJS uses this polyfill: https://stenciljs.com

There's no mention of custom-elements or document-register-element in Stencil's package.json.

Google AMP pages use this polyfill: https://www.ampproject.org

They hide the constructors away, end users only ever use the pre-defined custom elements. Well, if they retrieve the classes from customElements (does this even work with native v0?), then those constructors accept no args, so no problems there.


In my case, none of the strategies they use work because of how I want to handle constructors. I think I solved it now though, it wasn't that hard after all. Thanks for all the advice!

trusktr commented 7 years ago

If this is confirmed I think I should update the README with Rollup specific hint.

Yep, I think it's backwards in rollup-plugin-babel.

trusktr commented 7 years ago

All these little things! So much tinkering needed.