Should static initializers see the decorated or undecorated version of the class?

[rbuckton]

In https://tc39.github.io/proposal-decorators/#runtime-semantics-class-definition-evaluation, step 34 assigns the binding for className to F in the class body, prior to any replace hooks being evaluated in step 36. Is this intended effect? In TypeScript, we addressed feedback that the binding inside the class body would be the decorated class, otherwise you might see issues:

@decoratorWithReplaceHook
class C {
  clone() {
    const obj = new C(); // !!! Undecorated C
    // ... set properties
    return obj;
  }
}

However we would also have to address how that would affect static initializers:

@decoratorWithReplaceHook
class C {
  static instance = new C(); // !!! Undecorated C
}

Should instance be the undecorated C or the decorated C? If it needs to be the decorated C, how do we deal with "finish" hooks that freeze the class?

[littledan]

How does TypeScript solve this problem? Finish hooks are supposed to run after the static initializers run (per the Munich 2016 consensus). Do you want to run replacers before static initializers, and finishers afterwards?

[rbuckton]

Given the following:

@decorator
class C {
  static instance = new C();
  clone() { return new C(); }
}

Typescript emits the following:

var __decorate = ...
var C_1;
let C = C_1 = class C {
    clone() { return new C_1(); }
};
C.instance = new C_1();
C = C_1 = __decorate([
    decorator
], C);

Static initializers currently get the undecorated C, while inside the class body we use the decorated C (the C_1 alias is to preserve the class name of "C", as using Object.defineProperty to change the class name caused issues in some runtimes and broke TS -> Babel -> JS toolchains).

If you wanted instance to be a decorated C, you would have to make it an accessor:

let _instance;
class C {
  static get instance() { return _instance || (_instance = new C()); }
  clone() { return new C(); }
}

As to whether replacers should run first, that's an interesting question. There are possibly two different desired outcomes, given two different use cases.

In one case, for static instance = new C(), I might expect instance to be an instance of the decorated C. However, consider the following decorator:

/** replace object with a noop if an environment key is not set */
function conditional(key, value) {
  return descriptor => {
    if (process.env[key] === value) return;
    descriptor.extras = [{ kind: "hook", placement: "static", replace: () => noop }];
  };
}
function noop() {}

...

@conditional("NODE_ENV", "debug")
class C {
  static instance = new C();
}

I wouldn't want each use of @conditional to clutter noop. I can't freeze noop since that would result in exceptions in strict mode. In the case of conditional, it could possibly be re-written to produce a Proxy to trap and ignore these things, but there could be other similar cases of replacers that return a singleton.

[ljharb]

If the class is replaced, why would the class body (which is potentially no longer in existence) reference the decorated C (the replacement)?

In other words, I think I'd expect the outer C binding to be the decorated one, but the inner C binding to be the undecorated one. If the replacer wants static instance to be an instance of the decorated class, then it seems like the replacement should provide its own instance property.

[zenparsing]

@replacing
class P {
  constructor(x, y) { this.x = x; this.y = y }
  add(other) { return new P(this.x + other.x, this.y + other.y) }
}

new P(0, 0).add(1, 1) instanceof P; // false?

[ljharb]

Indeed; if it's going to replace P successfully, it should probably have the knowledge that it needs to replace add as well.

[jsg2021]

I would expect that if i decorate a class with a decorator (and it replaces my class) any reference to that class (inside or out) be the decorated version.

[jsg2021]

Hmm, maybe not.

[rbuckton]

@ljharb: When we first implemented decorators, we did exactly that (the inner C was left alone). Early feedback was that this was not what users of the feature wanted. This is often because of how confusing the lexical binding for the class can be as it differs between class declarations, function declarations and function expressions. We found our users expected the C on both the inside and the outside of the class body to be the same after decoration. Without that, the only way to define the class and use it in a way like @zenparsing's example is like this:

let C = @decoratorWithReplaceHook
class {
  copy() { return new C(); }
}

While feasible, this is definitely not ergonomic.

[ljharb]

Could we make a replaced class whose body contains a reference to its own identifier binding throw? That way perhaps we could address it with the class access proposal.

[rbuckton]

What would be the purpose of that? It seems like it would be better just to replace the binding, rather than make it unusable at all. It would also become a refactoring hazard. Why should adding a decorator to my class require me to completely rewrite it?

[ljharb]

How would we replace the binding in an initializer that’s already been evaluated prior to the replacement decorator?

Note I’m not suggesting any decorator - only one that replaces the class.

[rbuckton]

That would mean that code that works fine one day could break unexpectedly when a dependency changes to add a "replace" hook. I also don't know that the class access proposal is any kind of solution for this, as it would have the same issue as the lexical binding (in that it itself is also effectively a lexical binding for the class).

[ljharb]

yeah that's a fair point.

How would you proposal specifying a way for the replacement to alter the inner binding as well? Defer invocation of static initializers until after all decorators have ran?

[rbuckton]

That requires some thinking. I'm likely to be on the side of having static initializers run after replacers, since it would align with how instance initializers would evaluate (as an instance of whatever the decorated class becomes), with the caveat that something like the conditional decorator needs to take additional caution. I'm more likely to err on the class author/decorator consumer side than the decorator producer side, since developers will spend significantly higher percentage of time writing classes (including those that use decorators) than they will spend writing the decorators themselves.

[rbuckton]

The TypeScript emit (where static initializers run before decorators) is mostly due to the fact that Stage 1 decorators had no concept of "phases" when it comes to decorator evaluation, so you couldn't separate out constructor replacement from "I want to freeze the static side of the class" like you can with hook evaluation.

[jsg2021]

Thinking about an example of a replacement decorator... Say you have a react component. The component is decorated with a store connect decorator which composes the component. In this scenario, the result of the decoration is a wrapper component that renders the decorated one. It’s of a completely different heritage (tho it is still descendent of Component)

Every HOC decorator pattern i’ve seen proposed (and used) does not expect the class name to be the decorated one. However, I’m not sure if that’s a problem. (unless we cannot get a reference to the replaced class)

[ljharb]

It seems better to me to stick to the actual JS thing of, lexical bindings can't magically change out from under you while you're executing synchronously. In other words:

@replacing
class P {
  constructor(x, y) { this.x = x; this.y = y }
  add(other) { return new P(this.x + other.x, this.y + other.y) }
}

new P(0, 0).add(1, 1) instanceof P === false;

I think it makes sense that if I, the class author, want the P to point to "not the thing P lexically points to", I have to use the let P; approach (ie, make an unnamed class, and reference the created binding). This would be the same thing in a bound function (except that there's no current way to alter a declaration mid-flight)

@replacing
function f() { return f; }

assert(f() !== f);

@replacing
const f = function () { return f; }
assert(f() === f);

[rbuckton]

@replacing
function f() { return f; }

assert(f() !== f);

That would be incorrect, as function declarations do not declare a lexical binding inside their body:

> function f() { console.log("f"); f = function () { console.log("g"); } }
undefined
> f()
f
undefined
> f()
g
undefined
>

Function expressions however, do declare a lexical binding inside their body, if they have a name.

[ljharb]

ha, fair enough - so this really is a unique problem to class declarations combined with replacers :-/

I wonder why class declarations deviated from function declarations in this way - it'd have made this problem now much more straightforward :-)

[rbuckton]

Agreed, I recall raising this concern within the TypeScript team roughly 5 years ago (that the inner lexical binding for class declarations made no sense), but was not heavily involved in TC39 at the time. Outside of transpiled decorators in an ES6 or later environment, I imagine there are few if any cases where the inner lexical binding would be relevant. In my experience (and judging from the customer feedback that led to TypeScript's current handling of the lexical binding WRT decorators), most developers seem to assume class C {} behaves like function C() {}, so having the C inside and outside of the class body be different when decorators are applied would be surprising.

[ljharb]

Would it be worth pursuing a change to remove the inner binding on a class declaration, or is that likely to be web incompatible?

[rbuckton]

We would need data on that. We would still have the same odd behavior WRT named class expressions, however, though it would more closely align with the behavior of function expressions.

[rbuckton]

That said, in my experience the inner lexical binding has been more of a curse than a blessing, and any time I've come across a case where the fact the outer and inner binding mattered, it was usually the case that the inner lexical binding was worked around (as in the TypeScript emit above), rather than leveraged. However that's not enough data points for an accurate assessment.

[jsg2021]

More examples :) I'm starting to see two kinds of replacements. One is classical where the replacement is a subclass of the decorated, and the other is a completely different thing. (noop or component swap) like so:

import React from 'react';

@connect('somekey-in-state-store')
class Foo extends React.Component {

    static propTypes = {
        data: PropTypes.any // passed from connected wrapper
    }

    someMethod () {
        return Foo;
    }

    render () {
        return (
            <div />
        );
    }
}

assert(Foo.propTypes.data === undefined)
assert(Foo.prototype.someMethod === undefined)

[littledan]

We talked about this in the decorators call. Would running replace hooks before static field initializers, and finishers after, solve this issue? This alternative is specified in #232.

[littledan]

OK, sounds like #232 is a bad idea; thanks to @nicolo-ribaudo for detailed analysis. Additionally, @zenparsing raised an important refactoring hazard for this change. I'll be thinking about how we can do something closer to TypeScript semantics with respect to scoping, since it seems like they hit on a good resolution.

Note that there's a lot going on with these bindings, so it will be a little complicated to fix. I don't think we can just remove inner bindings, as they are very important for, e.g., class expressions. I'm also a little uneasy to change the inner binding from immutable to mutable for decorated classes--we only need the system to be able to mutate it, not JS code. At the same time, I imagine if you have an arrow function in a static field initializer, it will eventually see the decorated class.

[pabloalmunia]

From my humble point of view, the decorators are only a way to add functionality orthogonally to a class and therefore everything we can do with decorators can be done with code inserted into in the class body.

When a constructor returns an object of another class, the static members of the class are not altered and can be called without problems.

class T {
  constructor() {
    return {a: 1}
  }
  static m() {
    console.log('static method');
  }
  z() {
    console.log('unreachable method')
  }
}

T.m();
const t = new T();
console.assert(!(t instanceof T));

With this type of construction we do not say "the class is changed", we understand that the constructor has decided to change the type of the instance of the object that it returns.

Analogously, when a decorator do a substitution of the constructor, the class is not changing as such, only is modifyed his constructor. It is possible that constructor returns an object of the class (it is making a wrapper around the constructor) or an object of another class or a function, but the class as such has not been modified, only its constructor has been modified.

More precisely, when we say that a decorator substitutes a class we should say that a decorator replaces the constructor of the class. It is a small difference, but the meaning changes substantially.

[littledan]

@pabloalmunia Interesting. Maybe we should give a way to wrap the constructor's functionality without wrapping the rest of the class. Among what we've discussed concretely, #232 is fairly close to this, but it still exposes more than just the constructor itself.

I'm not sure exactly what sort of object we'd expose for the wrapping, though, to be more minimal than #232. I'm also not sure what to do about the expectations that static fields be present on the thing that's being wrapped.

[littledan]

I've been giving this issue a bit of thought (apologies for the length of this post). It relates to a pretty core use case of decorators: Lots of people want to be able to write a decorator which replaces the entire class. This sort of thing would come up for function decorators as well (where anonymous function expressions have their own immutable inner binding, like classes).

@rbuckton suggested above eliminating the inner binding of classes. However, this inner binding has been widely implemented (in V8, JSC, SpiderMonkey, ChakraCore, TypeScript and Babel, going back to Babel 5.8.38 at least), and I'm not sure if it would be ecosystem-compatible (or desirable) to eliminate it.

I see two approaches to this problem in transpilers:

• In TypeScript (3.3.3333) and Babel 5.8.38, the generated code would mutate the inner class binding based on the output of the decorator.
• In Babel 6.26.3 and 7.3.3, the legacy decorator transform would leave inner references to the class

Have people been complaining about the Babel behavior? I don't see any issues about this in the appropriate label in the issue tracker, cc @nicolo-ribaudo .

Given this, I see three options:

1. Babel 6-inspired semantics/Normative: replacers -> static fields -> finishers #232: Run the replacer before static fields are added, keeping the class binding in TDZ while the decorator is running, and then immutably initialize it to the result of the decorator.
2. Babel 7-inspired semantics/the current proposal: Run the replacer after static fields are added, leaving the inner bindings pointing to the pre-decorator version of the class.
3. TS/Babel 5-inspired semantics: The fanciest choice is to make the inner class binding mutable, and have the decorator mutate it.

• Even fancier would be to make them fake "immutable" bindings that only get reset by decorators and never mutated by anything else.

I expect that we'd have a hard time getting consensus in TC39 on either of these proposals in option 3, since they feel a bit like playing fast and loose with JS semantics. It's not nice to make something change underneath your feet when it was previously stable.

@nicolo-ribaudo pointed out to be that, if we go with built-in decorators, we will have the opportunity to add multiple built-in decorators, for the semantics of both 1 and 2.

I looked into the @connect example a bit more, which would be pretty solidly broken by 1., but it seems that its usage as a decorator is not endorsed by official documentation and there is no code in the code base devoted to it; it just sort of falls out of legacy decorator semantics. I'm not sure if we have to support this particular case in our decorators MVP, as the documentation-encouraged usage pattern of calling connect as a function works just fine. It doesn't seem like a case where recursion is as important as in Ron's original post, so option 2. in a post-MVP built-in decorator may be a good solution.

So, I'd like to reconsider starting with option 1. It seems nice that it's a way to preserve expectations about recursion, and thinking about the function decorator case, it makes a lot of sense there too.

[jsg2021]

If the decorator replaces a class with an object or a function.... you would be initializing those static fields on a non-class? is that what i’m reading?

[ljharb]

I think it would be very bizarre to replace the class and then apply OldClass's fields to NewClass by default.

[nicolo-ribaudo]

Have people been complaining about the Babel behavior? I don't see any issues about this in the appropriate label in the issue tracker, cc @nicolo-ribaudo .

At least since I started working on decorators (about 1 year ago), we didn't receive any complaint. I also tried to search for older unlabeled issues, but I couldn't find anything.
Also, we have been supporting TypeScript in Babel for different months now and people started migrating to use Babel to compile TS (and TypeScript only to type-check): we didn't receive any complaint about the different behavior.

If I had to choose, I'd vote either for option 2 or option "without a number".

Adding static fields to the replacement seems wrong:

let result = 
@replaceWith(class B {})
class A {
  static foo = 2;
  bar = 3;
}

Why should result.foo be 2 but new result().bar not be 3?

[littledan]

@jsg2021 Yes.

@ljharb The above thread shows people finding each of these three alternatives counter-intuitive in some way or other, yet the feature remains highly desired. How do you think we should make this trade-off?

[nicolo-ribaudo]

Note that TypeScript provides the original class to static initializers, not the replaced one: #211 (comment)

What about a @rebind decorator, that when run after @replace updates the binding of the class? It would solve #211 (comment).

@wrap(f) is Babel 7, @wrap(f) @rebind is TypeScript.

[littledan]

Updating the binding is tricky territory, as we'd be making the immutable inner binding into a mutable one. However, this seems to be basically the only way to meet full TypeScript expectations. If people are OK with that, I like @nicolo-ribaudo 's decomposition. Let's start with just @wrap for the MVP and consider @rebind as a future decorator. (Nit, I'd probably write it as @rebind @wrap(f), as we want the rebinding to happen after the wrapping, and decorators run from the inside out.)

[jsg2021]

in the case of the replacement pattern, I think @nicolo-ribaudo 's example above, with A and B, are what I would expect. It should be the authors responsibility to hoist/copy/declare statics on to the resultant. (B) I would not expect the statics of A to be applied to B...and to add, I would expect order of decorators to matter, so I would expect what statics on the input (A) to exist within the replacement decorator.

[littledan]

@jsg2021 Are you OK with methods inside the class always seeing the original class, before the decorator which replaced the class is applied?

[jsg2021]

@jsg2021 Are you OK with methods inside the class always seeing the original class, before the decorator which replaced the class is applied?

Yes? I would even go further to say, I would expect/want that.

A's statics, as well as fields and such only see A instances. Outsiders see B. Only B can see A. B is not A, nor expected to have any relation to A. B composes A.

[tjcrowder]

For any coming to this issue late, as I did, let me spell out some things that are all in the above, but scattered here and there and sometimes somewhat assumed. This means that if you have this function:

function extendWithNiftyFeature(cls) {
    return class extends cls {
        niftyFeature() {
            console.log("Nifty feature called");
        }
    }
}

(perhaps niftyFeature needs access to super, so it can't just be added to cls.prototype)

and you have this

@wrap(extendWithNiftyFeature)
class X {
    methodOne() {
        this.niftyFeature();
    }
    methodTwo() {
        new X().niftyFeature();
    }
}

then this happens:

const x = new X();
x.methodOne(); // "Nifty feature called"
x.methodTwo(); // TypeError: (intermediate value).niftyFeature is not a function

If that error seems strange to you (as it did to me at first), it's useful to realize that X = extendWithNiftyFeature(X); exhibits the exact same behavior, today:

class X {
    methodOne() {
        this.niftyFeature();
    }
    methodTwo() {
        new X().niftyFeature();
    }
}
X = extendWithNiftyFeature(X);
const x = new X();
x.methodOne(); // "Nifty feature called"
x.methodTwo(); // TypeError: (intermediate value).niftyFeature is not a function

The X inside class X { /*...*/ } is an inner scope, immutable binding. It's not the same X that is created in the outer scope (although it has the same value that outer scope binding initially has).

This is consistent with named function expressions, which create an immutable inner binding for the function name

"use strict";
let foo = function foo(...args) {
    if (args.length === 0) {
        console.log("Calling foo(1)");
        foo(1);
    } else {
        console.log(`foo got ${JSON.stringify(args)}`);
    }
};
let bar = foo;
foo = function() {
    console.log("New foo");
};
bar();
// =>
// Calling foo(1)
// foo got [1]

(Function declarations don't create the immutable inner binding, so if it were a declaration, you'd see "New foo" instead of "foo got [1]"; the foo inside the function body is using the outer scope binding.)

So the upshot is that having @wrap behave this way is consistent with the simple mental model that the explainer provides for @wrap when applied to a class:

@wrap(f)
class C { }

is roughly equivalent to:

class C { }
C = f(C);

[littledan]

This question remains a hot topic; see #329 for later discussion. Personally, I suspect that this issue makes it hard to support class replacement, though maybe there's some way to decorate just what happens when the constructor runs (rather than the class's identity).

[pzuraq]

This has been resolved in the latest version of the proposal, static initializers see the decorated version of the class.