Decorators don't work correctly on classes merged with modules

[SimoneGianni]

Given the following code :

export = Stuff;

class Stuff {
  @decorator(Stuff.Specific)
  something :Stuff.Specific = new Stuff.Specific();
}

module Stuff {
  export class Specific {
    //...
  }
}

Due to how merged classes and modules are emitted, and due to the fact that decorators are executed quite early, the decorator will receive undefined instead of the constructor of Stuff.Specific.

I do understand that this is a chicken and egg situation, but could still there be a way of resolving it by changing the order or the way in which class, modules and decorators are emitted? For example, since decorators could be dependant on class or module stuff, while nothing can be dependant on decorators, could decorators be emitted last with proper references to the class they're used on?

[SimoneGianni]

Just to clarify, this is the current emitted JS from the playground for the code above :

    var Stuff = (function () {
        function Stuff() {
            this.something = new Stuff.Specific();
        }
        __decorate([
            decorator(Stuff.Specific)
        ], Stuff.prototype, "something");
        return Stuff;
    })();
    var Stuff;
    (function (Stuff) {
        var Specific = (function () {
            function Specific() {
            }
            return Specific;
        })();
        Stuff.Specific = Specific;
    })(Stuff || (Stuff = {}));
    return Stuff;

The call to __decorate are inside the anonymous function that create the Stuff type. If however they could me moved as the last thing before the return, they would still work as normally expected, plus also work for merged module classes :

    var Stuff = (function () {
        function Stuff() {
            this.something = new Stuff.Specific();
        }
        return Stuff;
    })();
    var Stuff;
    (function (Stuff) {
        var Specific = (function () {
            function Specific() {
            }
            return Specific;
        })();
        Stuff.Specific = Specific;
    })(Stuff || (Stuff = {}));
    // moved here
    __decorate([
        decorator(Stuff.Specific)
    ], Stuff.prototype, "something");
    return Stuff;

I'm not into compiler code enough to understand if this is an extremely hard to implement change or something trivial.

[SimoneGianni]

As totally expected, it also affects metadata collected using emitDecoratorMetadata.

[SimoneGianni]

In general it affects all forward references also inside a simple module :

module C {
  export class A {
    @dec(B) 
    prop :B = new B();
  }
  export class B {

  }
}

Also the metadata of A.prop will be wrong, cause it can't resolve B at that point.

[SimoneGianni]

I forked TS and tried to play a bit with it, and moving the decorators to the bottom of the emitted file seems to "fix" the simplest cases.

It's just a couple of hours of coding on a code base I had never seen before, so don't expect anything usable, but it's here : SimoneGianni@469f893

However, even if moving them to bottom of the file was a solution, some code inside the source file could try to execute something that depends on those decorators having been already executed.

How they are placed now is the right place to correctly isolate class creation and decoration in a single place, but makes it impossible to use types that are not already declared, not even for metadata.

[mhegazy]

//CC: @rbuckton

[mhegazy]

more information can be found at #4521

[SimoneGianni]

Issue #4521 starts from circular dependencies in module inclusion, which is another big issue, but everybody developing in JS (or C# or many other languages) is already used to deal with.

This issue is about the same problem, but inside a single file, which is something less expected; and given the "hidden" nature of decorators and metadata, is even more surprising to the average user.

A partial solution could be to have the TS compiler at least report that the given parameter is not resolvable in that position

[SimoneGianni]

I refactored my code to also accept a function returning the constructor, and to resolve it later, as suggested by @rbuckton to @pleerock in #4521.

However, this worked because in my case I can delay the resolution to later. If my decorator had to really decorate its target based on the value of the parameter, then it would not be possible to refactor it.

From my point of view, which is obviously partial and narrower than the one you have, it's still a better solution to move the __decorate calls "as later as possible to make sure parameters are resolved", assuming that if user code try to access decorator informations before they are defined, probably it would incur in other problems as well or would not be able to resolve the decorator anyway.

[mhegazy]

The current behavior matches the design of the feature. the solution would be to convert the decorator metadata helpers to getters, but that would be a breaking change.

[danquirk]

@mhegazy Could we at least report an error here then rather than have the decorator silently fail?

[mhegazy]

An error would not be correct in all cases. what if your decorator never used the type information, the error is not relevant.. what if it only used the return type, should we validate the parameter types?

this is a typical TDZ issue; and i think a reasonable fix is to switch to getters, that means more overhead on access, and breaking change.

[SimoneGianni]

@mhegazy true if we're talking about the metadata, but not if the decorator is asking for a type in it's own signature.

I mean, if I have a decorator :

@myDecorator(TypeDeclaredLater)

Even if them the decorator does not use the parameter, same as any other function, it should be reported as an error if it can't be resolved.

[SimoneGianni]

But the error reporting is another issue, it happens for every static call (decorators are actually "static calls"), also when the compiler could report the error, like not using external modules but simply inside the same code block.

Try to paste this in the playground :

module C {  
  export class A {
      static bProp = new B();
  }
  export class B {
  }
}

It will produce a code that can't work at runtime, but the compiler is not reporting any error, while it could cause it's obvious it will not work.

[mhegazy]

Passing a class explicitly to a function call, regardless if it is a decorator or not, is a different issue. I think this is a fair case for the compiler to detect and warn about. Using compiler jargon, this is definite assignment analysis; e.g. Using a variable before its definition, or variable used before definitely initialized. This is tracked by #21.

[rbuckton]

For classes, at least, we should perform the same analysis that we do for let/const as class declarations in ES6 are treated like let declarations, IIRC.

[mhegazy]

@sheetalnandi has a PR out with that already.