Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design:type metadata for cyclic dependencies throw at runtime #27519

Open
bajtos opened this issue Oct 3, 2018 · 13 comments

Comments

@bajtos
Copy link

commented Oct 3, 2018

TypeScript Version: 3.2.0-dev.20181003

Search Terms: design cyclic

Code

index.ts

import 'reflect-metadata';

function property() {
  return (target: Object, key: string) => {
    const t = Reflect.getMetadata('design:type', target, key);
    console.log('property %s#%s of type', target.constructor.name, key, t.name);
  }
}

class Product {
  // belongs to a category
  @property()
  category: Category;
}

class Category {
  // has many products
  @property()
  products: Product[];
}

tsconfig.json

{
  "$schema": "http://json.schemastore.org/tsconfig",
  "compilerOptions": {
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,

    "lib": ["es2018", "dom"],
    "module": "commonjs",
    "moduleResolution": "node",
    "target": "es2017"
  },
  "include": [
    "index.ts"
  ]
}

package.json

{
  "dependencies": {
    "reflect-metadata": "^0.1.12",
    "typescript": "^3.2.0-dev.20181003"
  }
}

Expected behavior:

Ideally, the code compiles and node index.js produces the following console output:

property Product#category of type Category
property Category#products of type Array

If this is not possible, then the compiler should detect cyclic dependencies and fail the compilation with a helpful error.

Actual behavior:

The code compiles. When the compiled code is executed, it fails at runtime.

index.js:23
    __metadata("design:type", Category)
                              ^

ReferenceError: Category is not defined
    at Object.<anonymous> (index.js:23:31)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    (...)

Here is the relevant snippet from the transpiled output:

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);

Playground Link:

Playground does not support emitDecoratorMetadata and experimentalDecorators options ☹️

Related Issues:

None found.

@bajtos

This comment has been minimized.

Copy link
Author

commented Oct 3, 2018

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);
@bajtos

This comment has been minimized.

Copy link
Author

commented Oct 4, 2018

RyanCavanaugh added the Design Limitation label 13 hours ago

@RyanCavanaugh Thank you for taking look at this issue. The description of Design Limitation label says: Constraints of the existing architecture prevent this from being fixed. I understand it may not be possible to make my code snippet shown above to work, but cannot we at least improve the compiler to detect the problem at compile time please? Is such improvement prevented by limitations of the existing architecture too?

@dawidgarus

This comment has been minimized.

Copy link

commented Apr 25, 2019

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.

@filipesilva

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

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.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I think this issue has been discussed and is indeed a design limitation - pinging @rbuckton for context.

@bajtos

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

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.

@waterplea

This comment has been minimized.

Copy link

commented Jun 11, 2019

That, or it shouldn't emit broken code.

Maybe you could add a warning that broken code was produced and remove it? After all, it is a metadata issue and the code itself is working. I came here from Angular, where metadata are only kept in dev mode, in production it works fine, but when I try to fire it up locally with JIT compiler — I get an error.

@spion

This comment has been minimized.

Copy link

commented Sep 20, 2019

@DanielRosenwasser do you think there is a way to move forward to introduce a forwardRef based implementation? Based on the above info it doesn't look like it would suffer from the issue. Of course it still wont work for constructor-based DI, but for lazy property based injection or for describing other relationships between classes it would work.

IMO this one might be worth the extra effort as its causing rippling effects in the new emergent TS ecosystem (angular, inversify/nestjs etc).

(In Inversify for instance, a type-unsafe token based alternative is recommended instead of forwardRef)

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

I'll try to keep it in mind, but I really don't think I can reliably give an answer in the near future until I can focus on decorators again. Recently my standards focus has been on optional chaining, nullish coalescing, and class fields.

@rbuckton

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@spion Any change we might make would be a breaking change. We considered switching to a new metadata format some time ago, as there is no way to handle forward references in a non-breaking way with the current emit. Something like this:

class Foo {
  // @__metadata("design:paramtypes", [Number, String])
  // @__metadata("design:returntype", Bar) // error
  @__metadata("design:typeinfo", {
    paramTypes: () => [Number, String], // deferred via arrow, no error
    returnType: () => Bar, // deferred via arrow, no error
  })
  method(x, y) {
    return new Bar(x, y);
  }
}
class Bar { ... }

We held off on any significant changes to this behavior as there are other issues limiting its utility:

  • No way to represent interfaces
  • No way to represent generics
  • No way to represent function/constructor types
  • No way to represent other complex types (tuples, arrays, conditional types, etc.)

With the decorators proposal still in flux, we have not prioritized any changes to decorator emit (including metadata) until the proposal progresses further along the standards track.

@spion

This comment has been minimized.

Copy link

commented Sep 22, 2019

Got it - I guess the new decorator proposal seems like a good time to revisit.

FWIW when we want run-time interface representation we currently use "classterfaces" i.e. interfaces defined as a class

class IMyInterface {
  prop!: number;
  method!: (arg: number) => number
}

They seem to work just fine as a substitute for interfaces:

class AddFive implements IMyInterface {
  prop = 5
  method(arg: number) { return this.prop + arg; }
}

and its easy to decorate them with plenty of additional info and/or use them as type-safe DI tokens.

@bajtos

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

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?

@spion

This comment has been minimized.

Copy link

commented Sep 30, 2019

@bajtos the problem is that any decorator causes an emit of metadata, but not all of them read it. This will cause non-related decorators to also crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.