Typescript: warning on emitDecoratorMetadata for interface in esnext

[staeke]

TypeScript Version: 2.4.2

Code
(using Angular 4)

// other-file.ts
export interface MyInterface { a: string }

// my-class.ts
import { Input } from '@angular/core'
import { MyInterface } from './otherFile'

class MyClass {
  @Input() myi: MyInterface;
}

tsconfig.json

{
  "compilerOptions": {
    ...
    "target": "es2017",
    "module": "esnext",
    "emitDecoratorMetadata": true,
    ...
  }
}

Expected behavior:
Working, no errors. I'd expect no type information emitted for interfaces, as they're only a Typescript compile construct.

Actual behavior:
Although this isn't a bug per se, TypeScript generates a decorator for type information, referencing MyInterface from otherFile. Only...this interface doesn't exist runtime. So bundlers (webpack in my case) produce a warning for this (something like otherFile doesn't export MyInterface). You can workaround this by creating a local type in my-class.ts. Or just accept warnings.

[mhegazy]

MyInterface should never be written to the generated .js. instead the compiler will use Object. emitDecoratorMetaData is a global switch, and adding it tells the compiler to try to write the metadata for decorated declarations. flagging these as errors means you can not decorate a method/property thorough out your code base unless it is of a class type.

[staeke]

Thanks for the reply @mhegazy. I understand that's the way it works. My hope was that I outlined such a reasonable case that either you'd

1. ...agree that it's bad to _try_ to emit metadata for interfaces, in which case this would be somewhere between request for improvement and bug report
2. ...be able to explain why it's reasonable with this behavior

Wouldn't it be much better to just not emit metadata for interface types then?

[aluanhaddad]

As @mhegazy stated, this would preclude decorating any members or member parameters that are not of class types.

As metadata emit is an optional and secondary consequence of the presence of a decorator in the source. Annotating all such members with meaningfully metadata emittable types would be prohibitively cumbersome and the resulting code would of far poorer quality in substantial number of cases.

There are a substantial number of cases where it is desirable to have metadata enabled but still decorate members they will not result in meaningful metadata but for which the behavior of the decorator is meaningful.

For example

import {autoinject, bindable} from 'aurelia-framework';
import {Router} from 'aurelia-router';

@autoinject export default class {
  constructor(readonly router: Router) {}

  @bindable model: {name?: string} = {};
}

In the above example, I need the metadata for router, but not model. However its presence on the latter is not harmful but if I were required to use a class for model I would simply stop using decorators as doing so would result in a less maintainable, less idiomatic program in this case

[mhegazy]

Wouldn't it be much better to just not emit metadata for interface types then?

we have no way of doing so at the moment. the emitMetaData uses the runtime value for serialization, this works for classes/enums/primitives, but not for interfaces. interfaces do not exist at run time.
Emitting interfaces means we have to either materialize interfaces as objects at runtime, or have some form of type serialization at runtime. you can find related discussion in https://github.com/Microsoft/TypeScript/issues/3628

[staeke]

Somehow I wasn't clear enough when describing my problem, which had to do with the wrong decorator reference being emitted. However, upon further investigation I actually realized that this is a problem with the webpack ts-loader plugin, and not the actual typescript compiler. So my apologies. I've raised the issue there https://github.com/TypeStrong/ts-loader/issues/613

[staeke]

My bad again. This really seems to be an issue with Typescript. I opened an issue with ts-loader here https://github.com/TypeStrong/ts-loader/issues/613#issuecomment-325309872 only to realize that they're just using the Typescript language service. So with my recent findings, let me summarize:

ts-loader yields different results when running these calls (the option transpileOnly in ts-loader determines which one to run)

languageService.getEmitOutput(filePath, ...)

```
compiler.transpileModule(fileContents, ...)

The first call generates
```js
__decorate([
    Input(),
    __metadata("design:type", Object)
], MyClass.prototype, "myi", void 0);

whereas the second generates

import { MyInterface } from './my-interface'
//...
__decorate([
    Input(),
    __metadata("design:type", typeof (_a = typeof MyInterface !== "undefined" && MyInterface) === "function" && _a || Object)
], MyClass.prototype, "myi", void 0);

As you can see, the second one references MyInterface, which, again, is just a compile time construct. It seems to me that transpileModule should yield the same results.

[staeke]

But of course, I realize that in order to know that it's just an interface you'd have to keep track of all the files. So maybe it's a big change to support. Maybe transpileModule could be part of the language service but now I'm a little bit out there.

[mhegazy]

The difference in emit is intentional. in single-fle-transpile mode (i.e transpileModule) the compiler has no way to know if the name is an interface or not. the emit is a runtime check for what the compiler does at design time if had the full program in memory. it should be functionally equivalent though in the case of interfaces.

[staeke]

First, let me just state that I'm really grateful for your work and that this may come off as fractious. I'm really not - just want Typescript to become as good as it can (and solve my problems of course :))

I understand that single-file transpile means that this error occurs. And that's how it's implemented. But it seems to me this case is an argument for that single-file-transpile is a bad option here. The way I see it, it only becomes an error if:

• you output ES6 modules (since require(...).MyInterface is ok, albeit undefined)
• you use decorators and emitDecoratorMetadata
• combine decorators (or constructor parameters) with an interface type

...i.e. not the ordinary use case, but not really esoteric either.

So given that this is the case, moving forward - what would we want it to be? I can see the following options. What is your stance/comment on these?

1. Single-file-transpile is and will be limited and can produce this type of error, which is expected. Then I'd suggest adding that, and possibly workarounds, to the documentation as a limiting factor
2. Transpile only is improved by keeping some kind of file cache of what is what (and more). This would most likely change the APIs
3. Add some dontTypeCheck (or similar) option to the getEmitOutput APIs. After all, probably the biggest use case for transpileOnly option is speeding up transpiling in day-to-day workflows. With this option the compiler would replace interfaces with objects in decoration, and possibly other things required for a valid transpilation, but not any other type checking.

[mhegazy]

Transpile only is improved by keeping some kind of file cache of what is what (and more). This would most likely change the APIs

that is not possible. the compiler does not have a way to examine all files, and there is no guarantee it will ever see all files in a single transpilation.

Add some dontTypeCheck (or similar) option to the getEmitOutput APIs. After all, probably the biggest use case for transpileOnly option is speeding up transpiling in day-to-day workflows. With this option the compiler would replace interfaces with objects in decoration, and possibly other things required for a valid transpilation, but not any other type checking.

Not sure how that solves the issue. we still have the single-file-transpile mode.

[mhegazy]

I think there is a bug here that we should address. the output currentlly is:

import { MyInterface } from './my-interface'

which is invalid ES6 import. the correct thing to do is to have an import to the module instead, i.e.

import * as MyInterface from './my-interface'

and then use MyInterface .MyInterface to check if the interface is undefined.

[staeke]

That indeed sounds like a great solution!

[mhegazy]

Discussed this with @rbuckton and seems that adding a synthesized import is a big change. the other alternative is to make this an error under --isolatedModules where:

• A type-only import is needed to emit meta-data
• --isolatedModules is set
• --module >= ES2015

[RoystonS]

Btw, Angular 2 uses this pattern quite a lot, e.g. to detect constructor parameter types. For interface types, it expects the developer to add an extra @Inject decorator to indicate the required concrete class or injection token, but for class types, there's enough information in the constructor parameter metadata to configure the dependency injector.

I've just tripped across this same issue when trying to use the transpileOnly: true option in the ts-loader webpack loader. The warnings about the bogus interface imports are problematic, but the generated runtime test code doesn't seem to be equivalent for non-interface types.

[staeke]

@RoystonS that is my use case as well

[aluanhaddad]

@RoystonS Angular's ahead of time compilation (AOT), which seems to be where that Community is going, doesn't even use TypeScript's standards based decorator implementation. They compile away the decorators in a process called lowering which brakes with what was the proposed standard which formed the basis for the TS implementation, and also the current proposed standard, in an incompatible manner.

[staeke]

@aluanhaddad Guesses about directions (with which I partly disagree btw) for Angular doesn’t affect that this is an issue with Typescript today, not ngc, not in the future.

[johnnyreilly]

I agree with @staeke - it does look like there's a problem today which it would be good to remedy. Let tomorrow's worries look after themselves :wink:

[olee]

Is there anything new on this issue?
I experienced this now myself and had to search for a while until I found this issue here and was able to fix the issue in my code.
For now I used a redefinition of the interface to prevent the incorrect emit:

interface Index_ extends Index { }

// ...

    @autobind
    private getRowHeight(index: Index_) {
        return ...;
    }

[olee]

I think this issue might be set to resolved with the upcoming release of Typescript 3.8 type imports.
Using those should always generate the correct code if I'm not wrong, am I?