TypeScript: Design:type metadata for cyclic dependencies throw at runtime

@rbuckton Any update on this?

I don’t really understand the reason why typescript emit “Type” instead of “() => Type” since the latter resolves some cyclic dependencies problems.

To me it sounds like a tiny change in an experimental feature anyway, and this tiny change would improve a lot of TS projects, and of course typescript itself.

Debugging circular import problem, when one is not aware of theses subtleties, is very hard.

Note: I spent some time yesterday making a minimal reproducible code and describing the issue in clear term here: https://github.com/microsoft/TypeScript/issues/41201.

I think this issue has been discussed and is indeed a design limitation

Personally, I can live with with the limitation, i.e. that TypeScript cannot emit design-type metadata when the property is using a type before it’s declared.

However, I’d like the problem to be detected and reported by the TypeScript compiler, the compiler should fail the build with a descriptive error message that will make it easy for the developer to understand what they did wrong and how to fix the problem.

Compare it with the current situation, where compiler happily produces JavaScript code and the problem is discovered only at runtime, usually with a hard crash on uncaught ReferenceError. I find such behavior rather impractical - what is the compiler good for when it cannot detect incorrect code? The problem can be also difficult to diagnose and troubleshoot, especially for people not aware of this limitation. The message “ReferenceError: Lock is not defined” with a stack trace pointing somewhere deep into code added by the compiler provide very little hints on how to fix the problem.

TypeScript compile could produce code that used forwardRef, similar how Angular suggest: https://angular.io/api/core/forwardRef Note that the Angular example doesn’t work when targeting esnext precisely because of this issue. The compiler could produce:

__metadata("design:type", __forwardRef(() => Category))

It also applies to design:paramtypes.

Any change we might make would be a breaking change. We held off on any significant changes to this behavior as there are other issues limiting its utility.

Makes sense 👍

In which case: Can we please improve the compiler to detect the situation when it’s going to emit invalid code that will later crash at runtime?

Would that be considering a breaking change? Since the emitted code is going to crash at runtime anyways, if the compiler decide to fail the build instead of emitting a code that will crash, then I would expect that such change should not break any valid/working applications out there.

Thoughts?

I was looking at this again today and @alxhub brought up an interesting point: TS will emit invalid ES2015 code for valid TS regardless of circular dependencies.

Consider this code:

function decorator(target) {}

// Error at compile time: error TS2449: Class 'Lock' used before its declaration.
// console.log(Lock);

@decorator
class Door {
  // No error when using Lock as a type, but will error at runtime
  //  __metadata("design:paramtypes", [Lock])
  //                                   ^
  // ReferenceError: Lock is not defined
  constructor(lock: Lock) { }
}

class Lock { }

Using TS 3.4.5 and NodeJS v10.10.0, you can compile and run it via npx tsc --target ES2015 --experimentalDecorators --emitDecoratorMetadata ./main.ts && node ./main.js.

The TS compilation will succeed but execution will fail:

kamik@RED-X1C6 MINGW64 /d/sandbox/rc-project/test (master)
$ npx tsc --target ES2015 --experimentalDecorators --emitDecoratorMetadata ./main.ts && node main.js
D:\sandbox\rc-project\test\main.js:22
    __metadata("design:paramtypes", [Lock])
                                     ^

ReferenceError: Lock is not defined
    at Object.<anonymous> (D:\sandbox\rc-project\test\main.js:22:38)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:696:3)

Yet there would be a compilation error if Lock was to be used as a value and not a type.

Perhaps there should be a compilation error for Types being used before being declared, at least if emitDecoratorMetadata is turned on. That, or it shouldn’t emit broken code.

A possible solution I see is to modify the code setting design-type metadata to use a resolver/getter function instead of passing the type constructor directly, and access design-type metadata a bit later, after all classes were defined. I guess such requirement makes my proposed solution rather impractical.

function property() {
    return (target, key) => {
      // Typically, users won't call process.nextTick but change the decorator to only store
      // design-type getter function. The actual type class will be read by other code at time 
      // that may be still in the same tick, but later enough to have all classes already defined
      process.nextTick(() => {
        const t = Reflect.getMetadata('design:type', target, key) || (() => {});
        console.log('property %s %s of type', target.constructor.name, key, t().name);
      });
    };
}
class Product {
}
__decorate([
    property(),
    __metadata("design:type", () => Category)
], Product.prototype, "category", void 0);
class Category {
}
__decorate([
    property(),
    __metadata("design:type", () => Array)
], Category.prototype, "products", void 0);