Decorators currently don't work with Babel

[43081j]

May be related to #205.

Essentially, given:

@property({ type: String })
prop = 'foo';

Babel configured with:

• @babel/plugin-proposal-decorators
• @babel/plugin-proposal-class-properties

It seems that one of the two results in this babel helper overwriting our property descriptor, so we lose the getter/setter and lit no longer is aware of changes.

I don't think it is a lit bug but it is something we need to track. I'm not sure which of the two babel plugins causes this as it needs a little more investigation (it looks to be used in the decorators plugin but specifically for class properties).

edit:

after further investigation it does look like #205, because it will return decorator(...) || descriptor. meaning as long as we return, it will be fixed.

[aadamsx]

Related #174

It's that decorators don't work in Babel7, I think they work in Babel6

[43081j]

There's two issues here with babel's output in reality. #206 doesn't seem to fix it...

• Babel overwrites the descriptor we set on the prototype but allows us to overwrite it if our decorator returns a descriptor
• Babel then overwrites the descriptor again in the constructor to initialise the class property, this time without caring what was there before

[mzeiher]

Hi, actually it should fix it (in a small test app I did (https://github.com/mzeiher/lit-element-test)) instead of assigning the property in the descriptor via Object.defineProperty on the targets' prototype within the decorator it deffers the assignment to the decorator transformation by babel by returning a new property descriptor (important part :)), luckily this is also supported by the typescript decorator transformation, strangely the default typedef does not allow to return a prop descriptor thus the "FixedPropertyDecorator" type

[mzeiher]

by looking at the TC39's proposal for decorators the same has to be done for stage-2, returning a new "patched" descriptor, so the general way of returning a new descirptor either alone or within an "new element descriptor" rather than applying it within the decorator looks kind of "future proof" if the spec does not change again :)

[43081j]

It is definitely correct to return a descriptor, no question there. This is how it'll be going forward.

But there is still a slight issue in babel's class properties plugin as you can see here.

This helper gets called in the constructor of our class. It'll modify the existing descriptor (which is why your fix works) but causes some strangeness because of it adding a value.

Anyhow i will review your PR as there's a few comments.

[mzeiher]

hi, the Object.defineProperty will not be called from the __initializerDefineProperty because the descriptor is null, babel calls _applyDecoratedDescriptor from the babel transform on the prototype and later on in the constructor _initializerDefineProperty with a null reference to descriptor thus not applying the descriptor with the value (if (!descriptor) return;) so it should work or did I miss something?

edit: the _applyDecoratedDescriptor returns null as a descriptor to indicate it already has defined the property foR the _initializerDefineProperty. The important step here is to return a new PropertyDescriptor in the decorator function without the "initializer" property from the babeled property descriptor :). This is also the reason why I keep the result of the initializer in a closure scope whee the getter for the property gets defined, otherwise you can't access it later on.
I hope you can make sense of the things I say, its evening in germany :D

[43081j]

_applyDecoratedDescriptor tries to return the finished descriptor, unless there's an initializer, then it will return null.

so its very much possible that a non-null descriptor gets returned (e.g. multiple decorators, with your fix in place) and the constructor modifies it.

[mzeiher]

ahh ok, multiple decorators are another topic to investigate, the winner to modify the property will be the first one defined which returns a descriptor without initializer, so if you implementing a descriptor you should always keep the descriptor which you get and (if setter/getters are defined) delegate to them in your own propertydesciptor implementations to keep the overrides in place from the other possible decorators.

edit: the property with the initializer prop is the one created by the babel transform which we don't want because we have or own implementation with get/set so in this case for the "property" decorator with a custom PropertyDescriptor returnned we should be on the safe side, -> for multiple descriptors things get weird.

thanks for input! my PR wasn't that thought out

[43081j]

multiple decorators is just one example, the issue i outlined does exist. there are many situations where a descriptor will be returned, maybe even the default behaviour after your proposed fix is introduced.

we are just lucky there are no immediately obvious side effects because the descriptor is modified to have a value rather than being completely replaced, and it seems that doesn't affect our getter/setter config.

babel does a few strange things here too... descriptors don't have an initializer (hence why you had to make a babel specific interface in your PR). non-legacy decorators do have an initializer, but their descriptor doesn't (still). they've essentially patched it in.

[mzeiher]

jeah, you're right. The questions is if its possible to support TS, Babel7 legacy and Babel7 Stage-2 (legacy=false) without side effects or just support the legacy mode for now, which is kind of the default at the moment and throw an error if argument[0].kind !== undefined or detect the stage-2 spec and instead of returning our own property desciptor return in the stage-2 case { ...argument[0], descriptor: } then it should work... unforunately we have no reference to the prototype constructor so this: proto.constructor as typeof UpdatingElement).createProperty(<prop>, options) } does not work anymore in stage-2.. there needs to be another way to create the property descriptor than rather then calling createProperty on the prop.constructor

[mjgchase]

My Babel 7 .babelrc if it helps , works for me

{
  "presets": [
    "@babel/preset-env"
  ],
  "plugins": [
    "@babel/plugin-syntax-dynamic-import",
    [
      "@babel/plugin-proposal-decorators",
      {
        "legacy": true
      }
    ],
    [
      "@babel/plugin-proposal-class-properties",
      {
        "loose": true
      }
    ]
  ]
}

[thepassle]

@mjgchase I believe this doesnt work when updating properties

[mjgchase]

@thepassle works fine for me , i.e

package.json

 "@babel/plugin-proposal-class-properties": "^7.1.0",
 "@babel/plugin-proposal-decorators": "^7.1.2",

import {LitElement, html, property} from '@polymer/lit-element';

export class ButtonToggle extends LitElement {

    @property({type: Array})
    buttons = null;

    @property({type: String})
    buttonClass = 'btn-default';

    constructor(){
        super();
        this.buttonClass = 'btn-primary'; <---------works and sets it in the view
    }

    _renderButtons(button) {
        return html`
                <button @click="${(e) => this.buttonClick(e, button)}" class="btn ${this.buttonClass}">${button.icon ? button.icon : button.text ? button.text : ''}</button>`
    }

    render() {
        return html`${this.buttons ? this.buttons.map(button => this._renderButtons(button)) : ''}`;
    }

    buttonClick(e, button) {
        e.stopPropagation();
        this.dispatchEvent(new CustomEvent('selected', {detail: button.value, bubbles: true}));
    }
}

window.customElements.define('button-toggle', ButtonToggle);

[thepassle]

@mjgchase Thats what I meant, consider this snippet:

import {LitElement, html, property} from '@polymer/lit-element';

export class ButtonToggle extends LitElement {

    @property({type: Array})
    buttons = [{icon:'',text:'foo'}];

    @property({type: String})
    buttonClass = 'btn-default'; // will set the default to 'btn-default'

    constructor(){
        super();
        this.buttonClass = 'btn-primary'; // will override btn-default and set it to 'btn-primary'
    }

    _renderButtons(button) {
        return html`<button @click="${(e) => this.buttonClick(e, button)}" class="btn ${this.buttonClass}">${button.icon ? button.icon : button.text ? button.text : ''}</button>`
    }

    render() {
        return this.buttons ? this.buttons.map(button => this._renderButtons(button)) : '';
    }

    buttonClick(e, button) {
        e.stopPropagation();
        this.buttonClass = 'btn-new-state'; // will update the value of this.buttonClass, but the change will not be picked up by lit, and will not trigger a rerender
        this.dispatchEvent(new CustomEvent('selected', {detail: button.value, bubbles: true}));
    }
}

window.customElements.define('hello-world', ButtonToggle);

Setting the value of this.buttonClass in the constructor kind of beats the purpose of setting it with the @property() decorator. Also, if you'd want to change the value of this.buttonClass by, for example, clicking on the button, Lit won't pick up the changes.

(Additionally, you didnt need the html tag in the render method)

[mjgchase]

@thepassle setting buttonClass in buttonClick still works only if I then run await this.requestUpdate() setting buttonClick as async. It must be setting the value otherwise the view would not update when calling requestUpdate. I always thought it was standard to call requestUpdate from an event?

(It's a WIP component and needed styles etc just used what I was currently on to show the point).

[thepassle]

Or just calling this.requestUpdate(). But again, that entirely beats the purpose of using the decorator

[mbrowne]

My comment on #205 may be of interest to those ready to upgrade to Babel 7.

[sorvell]

Dup of #156

[blikblum]

It's that decorators don't work in Babel7, I think they work in Babel6

Just tested with babel 6 + "transform-decorators-legacy" + "transform-class-properties" and also does not work. A issue similar to babel 7 + legacy option occurs: the property value is overwritten with a defineProperty call in constructor. This prevents the property setter configured by lit-element to be called

Seems that no configuration works with babel
babel6 / legacy
babel7 / legacy
babel7 / stage 2

So the only option for now to use decorators is with typescript

[JeremyBernier]

Here's the solution (after wasting hours on this):

In your babel config, set decoratorsBeforeExport: true

plugins: [
  ["@babel/plugin-proposal-decorators", { decoratorsBeforeExport: true }],
  ["@babel/plugin-proposal-class-properties", { "loose": true }]
]

[ediabal]

@JeremyBernier that doesn't seem to work for me. I still get the error for:

Support for the experimental syntax 'decorators-legacy' isn't currently enabled

Currently using...

"@babel/core": "^7.9.6",
"@babel/plugin-proposal-class-properties": "^7.8.3",
"@babel/plugin-proposal-decorators": "^7.8.3",
"@rollup/plugin-json": "^4.0.3",
"@rollup/plugin-url": "^4.0.2",
"@storybook/web-components": "^5.3.18",
"babel-loader": "^8.1.0",
"rollup": "^2.7.6",
"rollup-plugin-babel": "^4.4.0",
"rollup-plugin-node-resolve": "^5.2.0"

babel.config.js

module.exports = {
  presets: [
    ["@babel/env", { modules: false }]
  ],
  plugins: [
    ["@babel/plugin-proposal-decorators", { decoratorsBeforeExport: true }],
    ["@babel/plugin-proposal-class-properties", { "loose": true }]
  ]
};

[blikblum]

try ["@babel/plugin-proposal-decorators", { decoratorsBeforeExport: true, legacy: false }]

[ediabal]

@blikblum, thanks but still the same error.

[trusktr]

The problem with supporting both TypeScript and Babel 7 legacy decorators is that they have a number of differences.

What's the solution for writing a legacy decorator that work in both TypeScript and Babel?

EDIT: I removed some details from this comment (see my next comment for a link to a list of difference between Babel and TypeScript decorators)

[43081j]

Lit ships both now, it has two of every decorator to support both typescript and Babel under the hood.

Are you asking how to make your own that support both or do you think lit's don't work?

[trusktr]

Are you asking how to make your own that support both or do you think lit's don't work?

I was asking how to do it in general, but now I also inspected lit-element's:

After doing some research these past few days, it turns out there's actually not just two, but six configurations that decorator authors should ensure their decorators work in:

1. TypeScript experimentalDecorators with useDefineForClassFields set to false
2. TypeScript experimentalDecorators with useDefineForClassFields set to true
3. Babel with proposal-class-properties loose set to true and proposal-decorators legacy set to true
4. Babel with proposal-class-properties loose set to true and proposal-decorators legacy set to false (since Babel 7.1)
5. Babel with proposal-class-properties loose set to false and proposal-decorators legacy set to true
6. Babel with proposal-class-properties loose set to false and proposal-decorators legacy set to false (since Babel 7.1)

I started writing a list of differences between Babel and TypeScript legacy decorators at babel/babel#8864 (comment). If you know any more details to add, please do comment there.

I do see lit-element's decorators support both legacy and newer decorators ("newer" because there's even newer decorators that no tools support yet and plus the proposal has been put on pause in order to come up with a fourth form of decorators), but I haven't specifically tested it in all 6 of the above scenarios.

From a quick glance at

https://github.com/Polymer/lit-element/blob/2de92b532b8adad540b91f208063b2e15777029f/src/lib/updating-element.ts#L313-L334

it seems that lit-element decorators may fail in scenario 2, and possibly also in 5 and 6 because I think loose: false is basically equivalent to TypeScript's useDefineForClassFields: true, so the decorator accessors get overridden by new property descriptors on this (a.k.a. [[Define]] semantics).

Did I overlook if lit-element has an alternative for that case?

Lit-element's decorators don't return anything, so they won't run into the problem of [[Define]] semantics being used on class fields when legacy decorators return descriptors regardless of loose being set to true (tracked in babel/babel#12040).

I'm not sure if Babel is correct (always use [[Define]] semantics if legacy decorators return a descriptor, regardless of the class-properties loose option) or if TypeScript is correct (never use [[Define]] semantics even if a legacy decorator returns a descriptor, when useDefineForClassFields is false).

[43081j]

Lit's decorators don't work with define semantics. It's a well known issue being tracked, especially since typescript will soon enable such behaviour by default in new projects.

They are not expected to work with define semantics for now.

Nobody is "correct". The very latest proposal is correct and nobody implements that yet. So here we are in this funky no man's land with a bunch of differing implementations.

[trusktr]

Here's the MobX 6 proposal, which includes new decorators that work with [[Define]] but using them requires an extra step in each class' constructor. mobxjs/mobx#2325

Small MobX 6 example:

class Foo {
  @observable foo = 123
  constructor() {
    makeObservable(this)
  }
}
class Bar extends Foo {
  @observable bar = 456
  constructor() {
    super()
    makeObservable(this)
  }
}

With that pattern it doesn't matter if the fields use [[Define]] or [[Set]]. The makeObservable calls are required in each class of a class hierarchy, in order to override fields set with [[Define]].