Decorators stop working with typescript > 4.2.4

[CaptainCodeman]

Description

Using a version of Typescript later than 4.2.4 appears to stop some decorators working, query and property don't function, but customElement does

Steps to Reproduce

1. Write this code

import { html, LitElement } from 'lit';
import { customElement, property } from 'lit/decorators.js';

declare global {
  interface HTMLElementTagNameMap {
    "my-counter": MyCounterElement;
  }
}

@customElement('my-counter')
class MyCounterElement extends LitElement {
  @property() count: number = 0

  increment(e: Event) {
    this.count = this.count + 1
  }

  render() {
    return html`
      <button @click=${this.increment}>Increment</button>
      <p>Count: ${this.count}</p>
    `
  }
}

bundle with rollup:

'use strict';

import resolve from '@rollup/plugin-node-resolve';
import typescript from '@rollup/plugin-typescript';

export default {
  input: 'src/index.ts',
  output: {
    file: 'dist/index.js',
    sourcemap: true,
  },
  plugins: [
    resolve(),
    typescript({ typescript: require('typescript') }),
  ],
}

2. See this output...

Live Reproduction Link

https://stackblitz.com/edit/lit-html

Expected Results

Actual Results

Browsers Affected

• Chrome
• Firefox
• Edge
• Safari 11
• Safari 10
• IE 11

[justinfagnani]

@CaptainCodeman the repro link is to the base lit-html template right now.

[justinfagnani]

Usually when the decorators break it's because useDefineForClassFields is true in the tsconfig. Is there a tsconfig in this scenario, or is it provided by the Rollup plugin? Maybe a newer version of the plugin changed defaults?

[e111077]

related #1914

[CaptainCodeman]

The bundle config was as simple as possible:

tsconfig.json:

{
  "compilerOptions": {
    "sourceMap": true,
    "module": "esnext",
    "target": "esnext",
    "moduleResolution": "node",
    "experimentalDecorators": true,
    "lib": [
      "dom",
    ]
  }
}

rollup.config.js:

'use strict';

import resolve from '@rollup/plugin-node-resolve';
import typescript from '@rollup/plugin-typescript';

export default {
  input: 'src/index.ts',
  output: {
    file: 'dist/index.js',
    sourcemap: true,
  },
  plugins: [
    resolve(),
    typescript({ typescript: require('typescript') }),
  ],
}

Adding "useDefineForClassFields": false to tsconfig.json does appear to fix it even with a later version of Typescript (4.3.4)

[justinfagnani]

Something must have been changed in the Rollup plugin then, since this option is still false by default in TypeScript. I wonder if we should file a bug against the Rollup plugin, as true is a very unsafe default.

[abdonrd]

We have the same issue, and adding "useDefineForClassFields": false to tsconfig.json fix it.

[CaptainCodeman]

Searching for useDefineForClassFields in the rollup repo brings up nothing, so it doesn't appear that they are setting it 🤷🏻

[justinfagnani]

Yeah I noticed that too. It has to be set to true somehow if setting to false fixes the problem. Might be worth an issue anyway to see if someone there can help.

[KeithHenry]

@justinfagnani I think this is related to the janky version of Typescript Microsoft bake in to Visual Studio - we had the exact issue (all property decorators failing), it worked fine in production builds and VS Code, but anyone using Visual Studio 2019 and Typescript 4.2.4 got this error.

@abdonrd's suggestion to add "useDefineForClassFields": false (even though we didn't have it before) fixes it.

I've raised microsoft/TypeScript#45584 as it appears to be a bug in Typescript, not Lit.

[KeithHenry]

After much discussion on Typescript's issue forums it appears this change was very much by design, just undocumented. I've raise microsoft/TypeScript#45653 to address that, but the discussion (microsoft/TypeScript#45718) is very much about making this error report better, not fixing it.

With that in mind should we change the Lit documentation? If so there are two options:

• "useDefineForClassFields": false fixes the issue, but leaves class fields in the constructor and has issues with the new #private JS fields.

• Adding declare before the @property() field causes the specific field to not be emitted, so

 @property() declare count: number; // = 0 can't have field initialised any more

I think the former is much more practical for most users upgrading Lit projects to TS 4.3 (or 4.2.4 with VS2019) or later.

I think the latter is potentially best practice, as long term we want to emit JS fields for ES2022+ targets whenever we're not wrapping that with the decorator property get/set boilerplate.

However, the declared initialiser are no longer in the emitted constructor, so these will all start as undefined. Switching to declare for each property will break a lot of code (including @CaptainCodeman's example, as increment will set count to NaN).

[KeithHenry]

After a little testing with this I think there might be a good case for declare syntax as the best practice.

In older TSC or with "useDefineForClassFields": false @CaptainCodeman's example emits something like:

let MyCounterElement = class MyCounterElement extends LitElement { 
    constructor() { 
        super(...arguments); 
        this.count = 0; 
    } 
    //... rest of impl
}
__decorate([property()], MyCounterElement.prototype, 'count');

Since 3.7 with "useDefineForClassFields": true or since 4.3 with target: es2020 the same TS code emits as:

let MyCounterElement = class MyCounterElement extends LitElement { 
    count = 0; 
    //... rest of impl
}
__decorate([property()], MyCounterElement.prototype, 'count'); // Does nothing as count already defined!

This is a breaking change, but there's a quirk of the original implementation I don't think most users realise - count is created with a setter on the prototype, so this.count = 0; in the constructor actually goes through the property decorator.

This has two potential side effects:

• in a TS constructor implementation this.count will be undefined, not 0
• for each initialised field property will get called with an old value of undefined - this may create corner case bugs with custom hasChanged functions.

If we switch to declare syntax we have to make some changes to the TS:

@customElement('my-counter')
class MyCounterElement extends LitElement {
  @property() declare count: number; // declare ensures no JS emitted to stop __decorate, but also no initialiser

  increment(e: Event) {
    this.count = (this.count ?? 0) + 1; // fallback required to avoid set to NaN on first call
  }

  render() {
    return html`
      <button @click=${this.increment}>Increment</button>
      <p>Count: ${this.count ?? 0}</p>
    ` // fallback to avoid displaying empty value
  }
}

But I think these might be desirable, as we've avoided the potential side effects, and been explicit to TS that the property will be set up by the decorator annotation and not part of the class definition.

[hpx7]

Something must have been changed in the Rollup plugin then, since this option is still false by default in TypeScript. I wonder if we should file a bug against the Rollup plugin, as true is a very unsafe default.

Note that vite 2.5.0 now sets useDefineForClassFields to true by default vitejs/vite#4749

[abdonrd]

Related to this, I just see this in the esbuild v0.14.0 changelog:

TypeScript code follows JavaScript class field semantics with --target=esnext (#1480)

TypeScript 4.3 included a subtle breaking change that wasn't mentioned in the TypeScript 4.3 blog post: class fields will now be compiled with different semantics if "target": "ESNext" is present in tsconfig.json. Specifically in this case useDefineForClassFields will default to true when not specified instead of false. This means class field behavior in TypeScript code will now match JavaScript instead of doing something else:

class Base {
  set foo(value) { console.log('set', value) }
}
class Derived extends Base {
  foo = 123
}
new Derived()

In TypeScript 4.2 and below, the TypeScript compiler would generate code that prints set 123 when tsconfig.json contains "target": "ESNext" but in TypeScript 4.3 and above, the TypeScript compiler will now generate code that doesn't print anything. This is the difference between "assign" semantics and "define" semantics.

Previously you had to create a tsconfig.json file and specify "target": "ESNext" to get this behavior in esbuild. With this release, you can now also just pass --target=esnext to esbuild to force-enable this behavior. Note that esbuild doesn't do this by default even though the default value of --target= otherwise behaves like esnext. Since TypeScript's compiler doesn't do this behavior by default, it seems like a good idea for esbuild to not do this behavior by default either.

[sorvell]

Closing based on @abdonrd's last comment and the info in this thread. Please open a new issue with a specific repro if any problem or confusion around this issue remains. Thanks!