GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Implement the Stage 3 Decorators Proposal by rbuckton · Pull Request #50820 · microsoft/TypeScript

Conversation

@rbuckton

@rbuckton Ron Buckton (rbuckton) commented Sep 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This implements support for the Stage 3 Decorators proposal targeting ESNext through ES5 (except where it depends on functionality not available in a specific target, such as WeakMaps for down-level private names).

The following items are not currently supported:

  • --emitDecoratorMetadata, as metadata is currently under discussion in https://github.com/tc39/proposal-decorator-metadata and has not yet reached Stage 3.
  • Decorators on a declare field.
  • Parameter decorators will not be supported until a follow-on proposal has been adopted and has advanced to Stage 3.
    • See Parameter decorators tc39/proposal-decorators#47 for additional information.
  • Decorators may not change the type of the member or class they decorate. If we decide to allow this capability, we will do so in a later PR. This is under discussion in the following two issues:
    • Allow decorators to apply type guards to arguments of the method/function they decorate #50159
    • Support to method decorator that change the method signature #49229

With that out of the way, the following items are what is supported, or is new or changed for Decorators support in the Stage 3 proposal:

  • The --experimentalDecorators flag will continue to opt-in to the legacy decorator support (which still continues to support --emitDecoratorMetadata and parameter decorators).
  • ES Decorators are now supported without the --experimentalDecorators flag.
  • 🆕 ES Decorators will be transformed when the target is less than ESNext (or at least, until such time as the proposal reaches Stage 4).
  • 🆕 ES Decorators now accept exactly two arguments: target and context:
    • target — A value representing the element being decorated:
      • Classes, Methods, get accessors, and set accessors: This will be the function for that element.
      • Auto-Accessor fields (i.e., accessor x): This will be an object with get and set properties.
      • Fields: This will always be undefined.
    • context — An object containing additional context information about the decorated element such as:
      • kind - The kind of element ("class", "method", "getter", "setter", "field", "accessor").
      • name - The name of the element (either a string or symbol).
      • private - Whether the element has a private name.
      • static - Whether the element was declared static.
      • access - An object with either a get property, a set property, or both, that is used to read and write to the underlying value on an object.
      • addInitializer - A function that can be called to register a callback that is evaluated either when the class is defined or when an instance is created:
        • For static member decorators, initializers run after class decorators have been applied but before static fields are initialized.
        • For Class Decorators, initializers run after all static initializers.
        • For non-static member decorators, initializers run in the constructor before all field initializers are evaluated.
  • 🆕 ES Decorators can decorate private fields.
  • 🆕 ES Decorators can decorate class expressions.
  • ‼️ ES Accessor Decorators (i.e., for get and set declarations) no longer receive the combined property descriptor. Instead, they receive the accessor function they decorate.
    • A stage 1 proposal that expands upon auto-accessors to allow you to decorate get/set pairs can be found at https://github.com/tc39/proposal-grouped-and-auto-accessors.
  • ‼️ ES Member Decorators (i.e., for accessors, fields, and methods) no longer have immediate access to the constructor/prototype the member is defined on.
    • If you need access to the class constructor from a static member, you can use:
      context.addInitializer(function() { this /*constructor reference*/ });
    • If you need access to the instance (not the prototype) from a non-static member, you can use:
      context.addInitializer(function() { this /*instance reference*/ });
    • Non-static members currently have no way to access the constructor or prototype during class definition.
    • This behavior is under discussion at Suggestion: Allow member decorators to add both static and instance *extra* initializers. tc39/proposal-decorators#465.
  • ‼️ ES Member Decorators can no longer set the enumerable, configurable, or writable properties as they do not receive the property descriptor. You can partially achieve this via context.addInitializer, but with the caveat that initializers added by non-static member decorators will run during every instance construction.
  • When the name of the class is inferred from an assignment, we will now explicitly set the name of the class in some cases.
    This is not currently consistent in all cases and is only set when transforming native ES Decorators or class fields. While we generally have not strictly aligned with the ECMA-262 spec with respect to assigned names when downleveling classes and functions (sometimes your class will end up with an assigned name of class_1 or default_1), I opted to include this because name is one of the few keys available to a class decorator's context object, making it more important to support correctly.

Type Checking

When a decorator is applied to a class or class member, we check that the decorator can be invoked with the appropriate target and decorator context, and that its return value is consistent with its target. To do this, we check the decorator against a synthetic call signature, not unlike the following:

type SyntheticDecorator<T, C, R> = (target: T, context: C) => R | void;

The types we use for T, C, and R depend on the target of the decorator:

  • T — The type for the decoration target. This does not always correspond to the type of a member.
    • For a class decorator, this will be the class constructor type.
    • For a method decorator, this will be the function type of the method.
    • For a getter decorator, this will be the function type of the get method, not the type of the resulting property.
    • For a setter decorator, this will be the function type of the set method, not the type of the resulting property.
    • For an auto-accessor field decorator, this will be a { get, set } object corresponding to the generated get method and set method signatures.
    • For a normal field decorator, this will always be undefined.
  • C — The type for the decorator context. A context type based on the kind of decoration type, intersected with an object type consisting of the target's name, placement, and visibility (see below).
  • R — The allowed type for the decorator's return value. Note that any decorator may return void/undefined.
    • For a class, method, getter, or setter decorator, this will be T.
    • For an auto-accessor field decorator, this will be a { get?, set?, init? } whose get and set correspond to the generated get method and set method signatures. The optional init member can be used to
      inject an initializer mutator function.
    • For a normal field decorator, this can be an initializer mutator function.

Method Decorators

class MyClass {
    m(): void { ... }
}

A method decorator applied to m(): void would use the types

type T = (this: MyClass) => void;
type C = & ClassMethodDecoratorContext<MyClass, (this: MyClass) => void>
         & { name: "m", private: false, static: false };
type R = (this: MyClass) => void;

resulting in a call signature like

type ExpectedSignature = (
    target: (this: MyClass) => void,
    context: & ClassMethodDecoratorContext<MyClass, (this: MyClass) => void>
             & { name: "m", private: false, static: false },
) => ((this: MyClass) => void) | void;

Here, we specify a target type (T) of (this: MyClass) => void. We don't normally traffic around the this type for methods, but in this case it is important that we do. When a decorator replaces a method, it is fairly common to invoke the method you are replacing:

function log<T, A extends any[], R>(
    target: (this: T, ...args: A) => R,
    context: ClassMethodDecoratorContext<T, (this: T, ...args: A) => R>
) {
    return function (this: T, ...args: A): R {
        console.log(`${context.name.toString()}: enter`);
        try {
            // need the appropriate `this`
            return target.call(this, ...args);
        }
        finally {
            console.log(`${context.name.toString()}: exit`);
        }
    };
}

You may also notice that we intersect a common context type, in this case ClassMethodDecoratorContext, with a type literal. This type literal contains information specific to the member, allowing you to write decorators that are restricted to members with a certain name, placement, or accessibility. For example, you may have a decorator that is intended to only be used on the Symbol.iterator method

function iteratorWrap<T, V>(
    target: (this: T) => Iterable<V>,
    context: ClassMethodDecoratorContext<T, (this: T) => Iterable<V>> & { name: Symbol.iterator }
) {
    ...
}

, or one that is restricted to static fields

function lazyStatic<T, V>(
    target: undefined,
    context: ClassFieldDecoratorContext<T, V> & { static: true }
) {
    ...
}

, or one that prohibits usage on private members

function publicOnly(
    target: unknown,
    context: ClassMemberDecoratorContext & { private: false }
) {
    ...
}

We've chosen to perform an intersection here rather than add additional type parameters to each *DecoratorContext type for several reasons. The type literal allows for a convenient way to introduce a restriction in your decorator code without needing to fuss over type parameter order. Additionally, in the future we may opt to allow a decorator to replace the type of its decoration target. This means we may need to flow additional type information into the context to support the access property, which acts on the final type of the decorated element. The type literal allows us to be flexible with future changes.

Getter and Setter Decorators

class MyClass {
    get x(): string { ... }
    set x(value: string) { ... }
}

A getter decorator applied to get x(): string above would have the types

type T = (this: MyClass) => string;
type C = ClassGetterDecoratorContext<MyClass, string> & { name: "x", private: false, static: false };
type R = (this: MyClass) => string;

resulting in a call signature like

type ExpectedSignature = (
    target: (this: MyClass) => string,
    context: ClassGetterDecoratorContext<MyClass, string> & { name: "x", private: false, static: false },
) => ((this: MyClass) => string) | void;

, while a setter decorator applied to set x(value: string) would have the types

type T = (this: MyClass, value: string) => void;
type C = ClassSetterDecoratorContext<MyClass, string> { name: "x", private: false, static: false };
type R = (this: MyClass, value: string) => void;

resulting in a call signature like

type ExpectedSignature = (
    target: (this: MyClass, value: string) => void,
    context: ClassSetterDecoratorContext<MyClass, string> & { name: "x", private: false, static: false },
) => ((this: MyClass, value: string) => void) | void;

Getter and setter decorators in the Stage 3 decorators proposal differ significantly from TypeScript's legacy decorators. Legacy decorators operated on a PropertyDescriptor, giving you access to both the get and set functions as properties of the descriptor. Stage 3 decorators, however, operate directly on the get and set methods themselves.

Field Decorators

class MyClass {
    #x: string = ...;
}

A field decorator applied to a field like #x: string above (i.e., one that does not have a leading accessor keyword) would have the types

type T = undefined;
type C = ClassFieldDecoratorContext<MyClass, string> & { name: "#x", private: true, static: false };
type R = (this: MyClass, value: string) => string;

resulting in a call signature like

type ExpectedSignature = (
    target: undefined,
    context: ClassFieldDecoratorContext<MyClass, string> & { name: "#x", private: true, static: false },
) => ((this: MyClass, value: string) => string) | void;

The target of a field decorator is always undefined, as there is nothing installed on the class or prototype during declaration evaluation. Non-static fields are installed only when an instance is created, while static fields are installed only after all decorators have been evaluated. This means that you cannot replace a field in the same way that you can replace a method or accessor. Instead, you can return an initializer mutator function — a callback that can observe, and potentially replace, the field's initialized value prior to the field being defined on the object:

function addOne<T>(
    target: undefined,
    context: ClassFieldDecoratorContext<T, number>
) {
    return function (this: T, value: number) {
        return value + 1;
    };
}

class C {
    @addOne
    @addOne
    x = 1;
}
new C().x; // 3

This essentially behaves as if the following happened instead:

let f1, f2;
class C {
    static {
        f1 = addOne(undefined, { ... });
        f2 = addOne(undefined, { ... })
    }
    x = f1.call(this, f2.call(this, 1));
}

Auto-Accessor Decorators

Stage 3 decorators introduced a new class element known as an "Auto-Accessor Field". This is a field that is transposed into pair of get/set methods of the same name, backed by a private field. This is not only a convenient way to represent a simple accessor pair, but also helps to avoid issus that occur if a decorator author were to attempt to replace an instance field with an accessor on the prototype, since an ECMAScript instance field would shadow the accessor when it is installed on the instance.

class MyClass {
    accessor y: number;
}

An auto-accessor decorator applied to a field like accessor y: string above would have the types

type T = ClassAccessorDecoratorTarget<MyClass, string>;
type C = ClassAccessorDecoratorContext<MyClass, string> & { name: "y", private: false, static: false };
type R = ClassAccessorDecoratorResult<MyClass, string>;

resulting in a call signature like

type ExpectedSignature = (
    target: undefined,
    context: ClassFieldDecoratorContext<MyClass, string> & { name: "#x", private: true, static: false },
) => ((this: MyClass, value: string) => string) | void;

Note that T in the example above is essentially the same as

type T = {
    get: (this: MyClass) => string,
    set: (this: MyClass, value: string) => void
};

, while R is essentially the same as

type R = {
    get?: (this: MyClass) => string,
    set?: (this: MyClass, value: string) => void,
    init?: (this: MyClass, value: string) => string
};

The return value (R) is designed to permit replacement of the get and set methods, as well as injecting an initializer mutator function like you can with a field.

Class Decorators

class MyClass {
    m(): void { ... }
    get x(): string { ... }
    set x(value: string) { ... }
    #x: string;
    accessor y: number;
}

A class decorator applied to class MyClass would use the types

type T = typeof MyClass;
type C = ClassDecoratorContext<typeof MyClass> & { name: "MyClass" };
type R = typeof MyClass;

resulting in a call signature like

type ExpectedSignature = (
    target: typeof MyClass,
    context: ClassDecoratorContext<typeof MyClass> & { name: "MyClass" }
) => typeof MyClass | void;

Fixes #48885

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chieffancypants, abdonrd, justinfagnani, dnalborczyk, trusktr, codeiotic, bondiano, Nerixyz, vdumbrav, btoo, and 4 more reacted with hooray emoji Vap0r1ze reacted with heart emoji whzx5byb, tonivj5, CarelessInternet, octet-stream, phiresky, shrujalshah28, HigherOrderLogic, ruojianll, lbguilherme, ProTip, and 24 more reacted with rocket emoji tonivj5, CarelessInternet, keatkeat87, L2jLiga, dnalborczyk, codeiotic, and vdumbrav reacted with eyes emoji
All reactions
  • 🎉 14 reactions
  • ❤️ 1 reaction
  • 🚀 34 reactions
  • 👀 7 reactions
Ron Buckton (rbuckton) added 10 commits September 2, 2022 13:28
@rbuckton
Support for auto-accessor fields
59162fd
@rbuckton
Add tests, ensure accessors are initialized in ctor
34da680
@rbuckton
classFields cleanup and PR feedback
02405ec
@rbuckton
Support for auto-accessor fields
9d1176a
@rbuckton
Add tests, ensure accessors are initialized in ctor
b853cb7
@rbuckton
Thread 'experimentalDecorators' through some utility functions
96c9393
@rbuckton
Split out decorators types from the default lib
fdfb49c
@rbuckton
Allow 'export default' before decorators
beb8e0a
@rbuckton
Clean up ts class transformation
f5c6935
@rbuckton
WIP Stage 3 Decorators
50c7e00
@rbuckton
Ron Buckton (rbuckton) requested review from Daniel Rosenwasser (DanielRosenwasser), Nathan Shively-Sanders (sandersn), Sheetal Nandi (sheetalkamat) and Wesley Wigham (weswigham) September 17, 2022 23:34
@typescript-bot TypeScript Bot (typescript-bot) assigned Ron Buckton (rbuckton) Sep 17, 2022
@typescript-bot TypeScript Bot (typescript-bot) added the Author: Team label Sep 17, 2022
@typescript-bot TypeScript Bot (typescript-bot) assigned Daniel Rosenwasser (DanielRosenwasser) Sep 17, 2022
@typescript-bot TypeScript Bot (typescript-bot) added the For Milestone Bug PRs that fix a bug with a specific milestone label Sep 17, 2022
@fatcerberus

Bruce Pascoe (fatcerberus) commented Sep 18, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

The --experimentalDecorators flag will continue to opt-in to the legacy decorator support

At some point this flag should probably be aliased/renamed to legacyDecorators or something, since “experimental” tends to imply “bleeding edge” and I can imagine future people unfamiliar with TS’s history blindly enabling it thinking they’re opting into something new and shiny as opposed to what it actually is, old and crusty. 😉

the thought process I’m imagining is essentially, “ooh, I like ES decorators, I wonder if this will give me even cooler decorator features…”

whzx5byb, markhaslam, dnalborczyk, trusktr, ruojianll, shivamd20, and LvChengbin reacted with thumbs up emoji
All reactions
  • 👍 7 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

At some point this flag should probably be aliased/renamed to legacyDecorators or something, since “experimental” tends to imply “bleeding edge” and I can imagine future people unfamiliar with TS’s history blindly enabling it thinking they’re opting into something new and shiny as opposed to what it actually is, old and crusty. 😉

Maybe aliased, but probably not renamed so as not to break existing consumers. Also, parameter decorators are still experimental.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fatcerberus

Bruce Pascoe (fatcerberus) commented Sep 19, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Also, parameter decorators are still experimental.

Yeah, my point was more that at some point we’re going to have a flag called “experimental” that opts into legacy behavior, and worse, legacy behavior that’s incompatible with the standard behavior that’ll be supported by default. It’s a weird state of affairs and I can definitely foresee the future GH issues “I enabled experimentalDecorators and all my existing decorators stopped working correctly, I thought this would just unlock additional features”

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ron Buckton (rbuckton) added 2 commits September 19, 2022 19:39
@rbuckton
Evaluation tests and bug fixes
86e0cd2
@rbuckton
Merge branch 'main' into decorators-stage-3
e87b89c
@rbuckton
Ron Buckton (rbuckton) force-pushed the decorators-stage-3 branch from a3ec9ee to e87b89c Compare September 20, 2022 03:56
@rbuckton
Set script target for esDecorator evaluator tests to ES2021
8c33a18
@rbuckton
Ron Buckton (rbuckton) force-pushed the decorators-stage-3 branch from 2d7e2b2 to 8c33a18 Compare September 20, 2022 17:02
@rbuckton
Merge branch 'main' into decorators-stage-3
3da06f6
@ruojianll

Lee (ruojianll) commented Sep 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

I have 2 questions:

  1. Why Non-static members currently have no way to access the constructor or prototype during class definition?
  2. Class decorator return a non-constructor value (like { }) is useful. Could you implement it?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Sep 25, 2022

Copy link
Copy Markdown
Contributor Author
  1. Why Non-static members currently have no way to access the constructor or prototype during class definition?

That is the current behavior of the proposal, but an alternative is being discussed in tc39/proposal-decorators#465.

  1. Class decorator return a non-constructor value (like { }) is useful. Could you implement it?

Class decorators can only return functions. You are welcome to open an issue at https://github.com/tc39/proposal-decorators if you believe this should be changed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jakebailey Jake Bailey (jakebailey) mentioned this pull request Sep 26, 2022
Sharp notation do not allow decorator .. #50602
Closed
@rbuckton
Ron Buckton (rbuckton) force-pushed the decorators-stage-3 branch from 831a5b2 to 673073c Compare January 18, 2023 22:49
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Jan 18, 2023
View reviewed changes
Ron Buckton (rbuckton) added 2 commits January 19, 2023 14:53
@rbuckton
Merge branch 'main' into decorators-stage-3
f0947fc
@rbuckton
Documentation comment updates
09855dc
ahejlsberg
Anders Hejlsberg (ahejlsberg) approved these changes Jan 19, 2023
View reviewed changes
@rbuckton
Ron Buckton (rbuckton) merged commit 5b18979 into main Jan 19, 2023
@rbuckton
Ron Buckton (rbuckton) deleted the decorators-stage-3 branch January 19, 2023 22:54
@rbuckton Ron Buckton (rbuckton) mentioned this pull request Jan 19, 2023
Add support for __esDecorate and related helpers microsoft/tslib#193
Merged
@robpalme

Rob Palmer (robpalme) commented Jan 20, 2023

Copy link
Copy Markdown

The ES2022 downlevel wraps the class in a function in order to provide encapsulated access to a couple of private static bindings (_staticExtraInitializers, _static_method_decorators).

class MyClass {
    @MyDecorator
    static method() {}
};

...is downlevelled to ES2022 like this...

let MyClass = (() => {
    let _staticExtraInitializers = [];
    let _static_method_decorators;
    return class MyClass {
        static {
            _static_method_decorators = [MyDecorator];
            __esDecorate(this, null, _static_method_decorators, { kind: "method", name: "method", static: true, private: false }, null, _staticExtraInitializers);
            __runInitializers(this, _staticExtraInitializers);
        }
        static method() { }
    };
})();

Would it be possible/desirable to specialize the emit for ES2022+ to use class private bindings? That would eliminate the function-wrapping and slightly improve the debugging experience (smaller callstack + in the object inspector # privates have less noise than closures).

class MyClass {
    static #staticExtraInitializers = [];
    static #method_decorators;
    static {
        this.#static_method_decorators = [MyDecorator];
        __esDecorate(this, null, this.#static_method_decorators, { kind: "method", name: "method", static: true, private: false }, null, this.#staticExtraInitializers);
        __runInitializers(this, this.#staticExtraInitializers);
    }
    static method() { }
};
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

Would it be possible/desirable to specialize the emit for ES2022+ to use class private bindings? That would eliminate the function-wrapping and slightly improve the debugging experience (smaller callstack + in the object inspector # privates have less noise than closures).

I considered this early on. Except for instance "initializers", those private fields would be unused after class definition evaluation and would take up extra space on the class itself. In addition, those temporary values can't be garbage collected. I'd argue they also make debugging worse when interrogating a Watch window given the excess properties attached to the class.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pflannery

Peter Flannery (pflannery) commented Jan 24, 2023

Copy link
Copy Markdown

I just tried this with 5.0.0-dev.20230123 and I'm seeing multiple decorator errors

error TS8038: Decorators must come after 'export' or 'export default' in JavaScript files.

I have tests that use a title decorator to override class names for test suite naming.

// foo.tests.js
@testTitle("foo bar")
export class FooTests {
// test functions...
}

Is this a bug or is there an option to allow this syntax for js?

JavaScript files

Does this mean typescript files support this syntax?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jakebailey

Jake Bailey (jakebailey) commented Jan 24, 2023

Copy link
Copy Markdown
Member

My understanding of the spec is that the above has to be written:

export
@testTitle("foo bar")
class FooTests {
// test functions...
}

So, we emit an error for the other form. But I believe for convenience, you can write it the "old way" in TypeScript code and it will emit it the other way around.

pflannery and lin72h reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

In a .ts file, you can have decorators either before or after export, since we previously parsed them before the export keyword in --legacyDecorators, and the Stage 3 proposal places them after the export (and default) keyword. In .js files, decorators must come after the export/default keyword to comply with the specification.

We still think that placing decorators after export is a poor design and have spoken with the proposal champion who intends to discuss this at the March TC39 meeting. We will likely ship with the JS support as it is specified for 5.0, and will amend it should the placement change in March. Yes, it would be a breaking change in JS files, but fixing it would be local to your project and wouldn't impact the packages you depend on or packages that depend on you. Also, while native implementations are currently underway, there are currently no shipping implementations of native decorators in any engine yet. This means we have a slightly longer window to discuss decorator placement.

uasan, pflannery, littledan, and seansfkelley reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@robpalme

Rob Palmer (robpalme) commented Jan 25, 2023

Copy link
Copy Markdown

I won't argue for using class privates in the implementation. There's just one bit of clarification.

Except for instance "initializers", those private fields would be unused after class definition evaluation and would take up extra space on the class itself. In addition, those temporary values can't be garbage collected.

In practice the storage costs and lifetimes are equivalent between class privates and closures. Due to a long-standing implementation detail in the way closures work in pretty much all engines, the lifetime of captured bindings matches the outer closure despite the fact that inner functions no longer have need for them. So with the current Decorators implementation, those temporary values (e.g. the static_method_decorators array) get captured by the class and have the same lifetime, even though we can see the inner static initializer does not have further use for them. Most DevTools now show you closure-captured bindings. Here's the retainers Chrome DevTools shows for the current Decorators implementation on the earlier code sample.

image

So if we want to release that memory earlier, we need to release it manually by setting it to undefined in either implementation. It's probably a small amount of memory, so whether it's worth the code to release it is your call.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asnaeb

Roberto De Lucia (asnaeb) commented Jan 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

I noticed that with the nightly builds, ClassFieldDecoratorContext does not provide typechecking anymore when only declaring void as decorator's return type

declare function stringField(_: undefined, ctx: ClassFieldDecoratorContext<A, string>): void | ((this: A, value: string) => string)
declare function stringFieldVoid(_: undefined, ctx: ClassFieldDecoratorContext<A, string>): void

declare class A {
    @stringField
    foo: number
    @stringFieldVoid // <- used to error, now doesn't anymore
    bar: number
}

Nightly Playground Link

The same code with a previous version of this PR:
v5.0.0-insiders.20230111 Playground Link

I did not test if this occurs with other DecoratorContext types and I don't know whether this is the desired behavior or not.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jan 25, 2023

Copy link
Copy Markdown
Member

Oh no - the removal of access means the second type param is unwitnessed when comparing types...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asnaeb

Roberto De Lucia (asnaeb) commented Jan 25, 2023

Copy link
Copy Markdown

Oh no - the removal of access means the second type param is unwitnessed when comparing types...

I have updated the comment. Declaring a full return type seems to fix this issue

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Jan 25, 2023

Copy link
Copy Markdown
Contributor Author

Oh no - the removal of access means the second type param is unwitnessed when comparing types...

I'd hate to remove the type param. The only other option I can think of would be to reintroduce access with get/set methods that aren't callable (i.e., unioning both possible definitions). As it is I'd just wait until after the January TC39 meeting and fix it then.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asnaeb

Roberto De Lucia (asnaeb) commented Jan 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Using the return type instead of ClassFieldDecoratorContext for type checking I am encountering a strange behavior. Consider the following code

declare function objectField<T extends Record<string, any>>
(_: undefined, ctx: ClassFieldDecoratorContext<A>): void | ((value: T) => T)

declare class A {
    @objectField
    foo: {
        a: string // <- Non-optional fields are not allowed here.
        b?: {
            c: number
        }
    }

    @objectField
    bar: {
        a?: string 
        b?: {
            c: number // <- Non-optional fields are allowed here.
        }
    }
}

To me, this looks inconsistent: Nightly Playground Link

This was not the case when using the type args on ClassFieldDecoratorContext as you can see here: v5.0.0-insiders.20230111 Playground Link

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jan 25, 2023

Copy link
Copy Markdown
Member

Oh no - the removal of access means the second type param is unwitnessed when comparing types...

I'd hate to remove the type param.

We could declare it as in out to enforce invariance even though it isn't witnessed yet.

interface ClassFieldDecoratorContext<This = unknown, in out Value = unknown>

Though I think originally, Value was effectively bivariant.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Jan 26, 2023
Tighter parameter decorator checking with --experimentalDecorators #52435
Closed
@Geekhyt 童欧巴 (Geekhyt) mentioned this pull request Jan 27, 2023
【第六十八期】2023-01-27 Geekhyt/weekly#72
Open
@nephix Thomas Pischulski (nephix) mentioned this pull request Jan 29, 2023
Reduntant type info MichalLytek/typegraphql-prisma#353
Open
@ste2425 Stephen Cooper (ste2425) mentioned this pull request Jan 31, 2023
Review what impact official ES Decorators will have moshegottlieb/ts-json-object#7
Closed
@asnaeb

Roberto De Lucia (asnaeb) commented Jan 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Oh no - the removal of access means the second type param is unwitnessed when comparing types...

I'd hate to remove the type param. The only other option I can think of would be to reintroduce access with get/set methods that aren't callable (i.e., unioning both possible definitions). As it is I'd just wait until after the January TC39 meeting and fix it then.

should we keep watching here for updates on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Feb 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I'd prefer people not continue discussing on the PR. The conversation is already very long, and it is hard to keep track of new issues.

I opened up #52540 to track the last-minute access member changes.

Any other questions/comments should be filed as new issues. Thanks all!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Feb 1, 2023
@typescript-bot TypeScript Bot (typescript-bot) added Fix Available A PR has been opened for this issue labels Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn Nathan Shively-Sanders (sandersn) sandersn approved these changes
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser approved these changes
@ahejlsberg Anders Hejlsberg (ahejlsberg) ahejlsberg approved these changes
@weswigham Wesley Wigham (weswigham) Awaiting requested review from weswigham
@sheetalkamat Sheetal Nandi (sheetalkamat) Awaiting requested review from sheetalkamat
+2 more reviewers
@JLHwung Huáng Jùnliàng (JLHwung) JLHwung left review comments
@uasan S.A.N (uasan) uasan approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser)

@rbuckton Ron Buckton (rbuckton)

Labels

Author: Team Fix Available A PR has been opened for this issue For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

TypeScript 5.0.0

Development

Successfully merging this pull request may close these issues.

Implement the updated JS decorators proposal

Uh oh!

There was an error while loading. Please reload this page.

19 participants

@rbuckton @fatcerberus @ruojianll @AaronFriel @DanielRosenwasser @typescript-bot @jakebailey @JLHwung @linbudu599 @asnaeb @hybrist @trusktr @ahejlsberg @KilianKilmister @robpalme @pflannery @sandersn @Jack-Works @uasan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.