useDefineForClassFields defaults to true when target is es2022, breaks decorators.

[justinfagnani]

Bug Report

useDefineForClassFields defaults to true when target is set to es2022 or esnext. This is breaking lots of projects that use decorators.

This was reported in #45584, but that issue was closed apparently because the title didn't include which versions of TypeScript set useDefineForClassFields and because the TypeScript playground doesn't exhibit this same behavior. I've included a reproduction for tsc here.

This behavior is not documented anywhere in the TypeScript docs that I can find.

🔎 Search Terms

useDefineForClassFields decorators

🕗 Version & Regression Information

Since 4.2.4? according to #45584
Present in 4.6.3 and 4.7.0-dev.20220422

⏯ Playground Link

This playground link does not demonstrate the problem, as it doesn't default useDefineForClassFields like tsc does. But if you set useDefineForClassFields to true you will see that that decorators could not work because the field is not an instance property that shadows the decorator defined prototype accessors.

Playground link with relevant code

This git repo reproduces the problem locally: https://github.com/justinfagnani/ts-decorators-bug

💻 Code

This does not work with target es2022:

const observe = (proto: Object, name: PropertyKey) => {
  const key = Symbol();
  Object.defineProperty(proto, name, {
    get() {
      return this[key];
    },
    set(v: unknown) {
      this[key] = v;
      this.onChange?.(name, v);
    }
  });
};


class A {
  @observe
  foo = 'abc';

  onChange(name: string, v: unknown) {
    console.log(`${name} changed to ${v}`);
  }
}

const a = new A();
a.foo = 'x'

🙁 Actual behavior

useDefineForClassFields defaults to true

🙂 Expected behavior

Option 1: useDefineForClassFields defaults to false like for other permutations of module and target until the new behavior is documented and users are notified with a prominent breaking change notice on the decorators documentation and the release notes.

Option 2: Use constructor initialization for decorated properties even if useDefineForClassFields is true. This will keep projects from breaking regardless of the config.

Option 3: Disallow both useDefineForClassFields: true and experimentalDecorators in the same config.

This behavior is breaking a large number of our users. Sometimes they are in control of their own tsconfig and can set useDefineForClassFields to false, but sometimes the config is generated for them via some other tool and they don't know how to set the value. In this case their project is just broken and they blame our library, not the update to TypeScript that included the undocumented breaking change.

[andrewbranch]

This behavior is not documented anywhere in the TypeScript docs that I can find.

In the tsconfig reference:

And in the release notes:

Open to adding something to the Decorators docs. We discussed other options in a design meeting and the only one we were sort of ok with was issuing a config diagnostic for experimentalDecorators. (In fact, this is exactly what I said here.)

[justinfagnani]

I guess I missed those and was looking in the decorator docs, don't know why I didn't see the 4.3 notes mention.

Why can't decorated fields opt-out of define semantics? We're seeing more and more users that are broken, and they get no errors at all. They're just assuming our library broke when it didn't change.

[andrewbranch]

Unfortunately the design notes only capture “That sounds horrifying to me” in answer to that question. From my memory, I think the sentiment was generally that experimental decorators are experimental, so we’re not going to break standards compliance elsewhere to make them keep working, and that users should simply not try to use them in combination with useDefineForClassFields, making the config diagnostic the only sensible mitigation.

[rbuckton]

useDefineForClassFields doesn't break "decorators", per se, they break this specific use of a decorator. Native decorators, per the Stage 3 spec, also cannot replace a field with an accessor. Instead, the Stage 3 decorators proposal introduces a new accessor keyword that performs the syntactic transformation of a field into an accessor for you:

class C {
  accessor foo = 'abc';
}
// becomes
class C {
  #foo_backing_store = 'abc';
  get foo() { return this.#foo_backing_store; }
  set foo(v) { this.#foo_backing_store = v; }
}

It will be a bit before TS support for Stage 3 Decorators is ready, but I have been considering introducing the accessor keyword to TS independently.

[rbuckton]

The other approach would be to recommend your users use declare if they have no control over the useDefineForClassFields setting:

class C {
  @observe
  declare foo: string; // field not declared on class, but decorator will still run

  constructor() {
    this.foo = "abc";
  }
}

declare on a field in TS does not introduce a runtime field declaration under useDefineForClassFields, it merely adds it to the type in TS. A declare field cannot have an initializer, however, so it must be moved to the body of the constructor.

[justinfagnani]

useDefineForClassFields breaks code that otherwise works with it set to false. Whether that's technically breaking decorators or not is pretty inside baseball that isn't so useful to our customers.

I know that we can tell users to use declare if they hit this problem, but we also tell them to set useDefineForClassFields to false. This doesn't help if they don't make their way to that documentation. More often their components silently don't work and they don't know why.

[rbuckton]

A property descriptor isn't provided to a field decorator because there's no descriptor to modify. This decorator breaks because it uses Object.defineProperty to introduce a new property that ends up shadowed by the field. Unfortunately, this was never something field decorators were designed to do. TS field decorators primarily existed as a means of recording metadata about a field. Performing mutations on the prototype outside of that capability is unsafe.

[typescript-bot]

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

[justinfagnani]

This is really unfortunate, IMO. The switching of this setting to default true in some cases is actively breaking our users.

[andrewbranch]

I tried to get some movement on this in another design meeting. I asked @rbuckton to update this issue with the summary of what we talked about; it looks like the design meeting notes aren’t up yet.