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

Does not work with mixins #5

Closed oravecz closed 7 years ago

oravecz commented 7 years ago

A very common pattern emerging when developing web components is the use of mixins. Whether using functional mixins or class based mixins, one does not typically code this way:

class MyElement extends HTMLElement

Instead, the mixin looks something like this:

import Composable from './composable'
class MyElement extends Composable(HTMLElement).compose( Mixin1, Mixin 2)

<composable.js>
export default ( Base ) => {

    class Composable extends Base { ... }

    return Composable
}

or

var MyElement = Mixin1(Mixin2(HTMLElement))

I'm not sure if the plugin can be written to work with these runtime semantics.

I have tried to create a subclass of HTMLElement called FixedHTMLElement which is passed in to Composable.

class FixedHTMLElement extends HTMLElement {}

class MyElement extends Composable(FixedHTMLElement).compose( Mixin1, Mixin 2)

But this didn't work. Are there any approaches that may work with your plugin for these cases?

WebReflection commented 7 years ago

wait a second ... your Composable breaks inheritance and you think it should be solved in here?

oravecz commented 7 years ago

Breaks how? Composable extends HTMLElement

I understand how your plugin can't pick up on this syntax. I am asking if there is a workaround.

WebReflection commented 7 years ago

OK, let me try again:

  1. I've no idea what's your Composable doing
  2. this plugin works with native classes
  3. you're using non native 3rd parts software that doesn't work anyway with transpiled code

What do you think I should do in here? I think the problem is in your utility that doesn't work well with transpiled code.

If you want me to even think about a solution, you have to give me your Composable source code because ...

one does not typically code this way ... ( show a class extending native class )

That's all I've ever done with classes and Custom Elements so ... yes, somebody does that, dunno what you do 'cause standard ES2015 inheritance, since we're talking about preset2015 compatibility, it's all that matters to me in here.

oravecz commented 7 years ago

Sorry, I thought that would be enough to demonstrate the point. Perhaps I was being too black/white with my intro. I should say that many developers of web components are discovering the power of mixins over the past year with libraries such as mixwith.js and articles from Component.Kitchen.

Here is the source for the ComposableMixin.

WebReflection commented 7 years ago

Can I have a whole page that shows there are issues once transpiled correctly with this plugin?

I need to be sure the right polyfill is used and the generated code is the expected one, or I won't be able to help.

oravecz commented 7 years ago

Sorry I haven't come back to this for a while. My problem with extending native classes was resolved by using https://github.com/webcomponents/webcomponentsjs/blob/master/custom-elements-es5-adapter.js.

Can you explain the difference between using this "adapter" versus the approach taken by your babel plugin?

WebReflection commented 7 years ago

you never mentioned Custom Elements native extends.

You could've used document-register-element that works already.

the wc adapter is a workaround/patch, not needed usually on my stacks

trusktr commented 7 years ago

@oravecz Yep, if you read about the caveat for document-register-element in it's README, then you don't need this transform to get Custom Elements v1 working. I think that that approach is better, if you can live with those caveats.

Can you explain the difference between using this "adapter" versus the approach taken by your babel plugin?

That adapter only works in Edge 12 and up. Good luck making a bundle that works in IE and Edge. If you don't need to support IE 11 or lower, then you'll be fine.

One way to do it (make a bundle that works in IE 11 and lower as well as Edge 12 and up) is to open up that adapter file (fork it) and wrap the class extends NativeHTMLElement definition in an eval() (otherwise IE 11 and lower throw a SyntaxError during parsing). You'll also have to convert arrow functions to normal functions, and for anything lower than IE 11 you need to replace Map with a polyfill.

This will make it works everywhere (IE and lower, Edge, and the others), but another problem is that some HTTP headers block eval from being used in any code.

If your website doesn't use such headers (CSP headers), then you'll have no problem.

However, if you're making something (f.e. a library as opposed to an app) that is designed to be used by others in who-knows-which websites, then they may face broken code if they have those headers enabled.

So far, using document-register-element is easier (if you want to make it work in older browsers) because it requires no special build handling for different browsers and doesn't require your users to have to think about "if this browser load this, if that browser load that". With document-register-element, you can make a single bundle that will simply work in all browsers (as long as you deal with the caveats) and your users don't have to worry about conditionally loading things for different browsers.