angular / angular-cli

CLI tool for Angular
https://cli.angular.io
MIT License
26.73k stars 11.98k forks source link

Warnings when using an exported interface in a class #2034

Closed Maistho closed 7 years ago

Maistho commented 8 years ago

1. OS

Arch Linux

Linux maistho-laptop 4.7.2-1-ARCH #1 SMP PREEMPT Sat Aug 20 23:02:56 CEST 2016 x86_64 GNU/Linux

2. Versions

angular-cli: 1.0.0-beta.11-webpack.8
node: 6.5.0
os: linux x64

3. Repro steps

I started a new repository, and added a new empty service, with an exported interface.

Complete changes, and a repo to pull and see the issue, can be found here:

https://github.com/Maistho/angular-cli-input-issue/commit/e5c483081bfd7af3dafd0db074179b3090668e27

After running ng build, I get two warnings in the Terminal

4. The log given by the failure

Hash: 354e709ba2cf588db59a                                                                                                                                                                                          
Version: webpack 2.1.0-beta.21
Time: 11161ms
            Asset       Size  Chunks             Chunk Names
   main.bundle.js    2.51 MB    0, 2  [emitted]  main
 styles.bundle.js    10.2 kB    1, 2  [emitted]  styles
        inline.js    5.53 kB       2  [emitted]  inline
         main.map     3.1 MB    0, 2  [emitted]  main
       styles.map    14.1 kB    1, 2  [emitted]  styles
       inline.map    5.59 kB       2  [emitted]  inline
       index.html  489 bytes          [emitted]  
assets/.npmignore    0 bytes          [emitted]  
chunk    {0} main.bundle.js, main.map (main) 2.46 MB {1} [initial] [rendered]
chunk    {1} styles.bundle.js, styles.map (styles) 9.96 kB {2} [initial] [rendered]
chunk    {2} inline.js, inline.map (inline) 0 bytes [entry] [rendered]

WARNING in ./src/app/app.component.ts
18:55 export 'TestInterface' was not found in './test.service'

WARNING in ./src/app/app.component.ts
18:88 export 'TestInterface' was not found in './test.service'
Child html-webpack-plugin for "index.html":
         Asset     Size  Chunks       Chunk Names
    index.html  2.82 kB       0       
    chunk    {0} index.html 357 bytes [entry] [rendered]

5. Mention any other details that might be useful.

I could not reproduce the issue without adding the @Input() to the class member.

Compiling with tsc does not produce any warnings.

hccampos commented 7 years ago

If you put all the interfaces alone in a single type (say a types.ts file) and then import them from there, you should get no errors.

The problem happens when there is some non-type-related code in a file mixed with some types and interfaces. In that case, webpack tries to import a file and look for a symbol in it but it doesn't exist. If the interfaces are all in a separate file, they are only used by TS for type checking and webpack doesn't care about it.

johnjerrico commented 7 years ago

Hmm, I did separate it; yes no error, but by the time i import the file, i could not reference the interface which i exported.. It's declared as undefined thus produce errors at later codes; am I missing something? On Thu, Apr 27, 2017 at 6:39 PM Hugo Campos notifications@github.com wrote:

If you put all the interfaces alone in a single type (say a types.ts file) and then import them from there, you should get no errors.

The problem happens when there is some non-type-related code in a file mixed with some types and interfaces. In that case, webpack tries to import a file and look for a symbol in it but it doesn't exist. If the interfaces are all in a separate file, they are only used by TS for type checking and webpack doesn't care about it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/angular/angular-cli/issues/2034#issuecomment-297689689, or mute the thread https://github.com/notifications/unsubscribe-auth/ACmP2QobYmOgxcKUOLSh9fbt6xgsdfljks5r0H5mgaJpZM4J5QId .

elvisbegovic commented 7 years ago

thanks but can't we fix it, i know it works with workaround but can't move all my interfaces in each module to one file. need fix

@hccampos

hccampos commented 7 years ago

@istiti to be honest, I don't see this getting fixed soon. It is not really an Angular CLI problem, and it is not really a Webpack problem. It is not a Typescript problem either... it is sort of in-between everything, and no one party can easily fix it, I think.

rolandjitsu commented 7 years ago

I'm curious if the CLI is using Rollup. I've experienced the same kind of issue on an Ionic 2+ project when I used Rollup instead of Webpack to bundle. So if the CLI uses Rollup, I think it might be where the source of this issue resides.

MattMorrisDev commented 7 years ago

Just FYI for everyone here, the warnings don't block the UI on 1.0.2 or 1.1.0-beta.0, so you can upgrade past 1.0.1 if you need to.

Snesi commented 7 years ago

Hi everyone, I'm having the same issue. I was following angular's styleguide by using classes instead of interfaces for type definitions, but decided to refactor and use interfaces instead and discovered the warning message.

However, now that I see that this hasn't been solved yet, I changed back to classes.

My question is, why use interfaces instead of classes? What's the real difference?

Why? A class can act as an interface (use implements instead of extends).

kylecordes commented 7 years ago

The difference between a class and an interface is this. The class says something about how the thing is constructed. What kind of code it has running behind it. Which may contain methods and so on. The interface only talks about the shape of data. So in terms of communicating to fellow developers the meaning of the code, and interface is the most clear way to talk about data your application does not construct and knows nothing about the construction of. For example, some data arriving over the wire as JSON then deserialized - it won't ever really be an instance of any class other than Object. But it may structurally match an interface.

TypeScript has an interesting "hack" around classes. If you define the class which only has fields, you can then treat it much as an interface. It will even pretend that an object is an instance of a class of which it is clearly not, the class only has fields (and therefore works like an interface). I think that is a horrible idea, I don't think it is a good feature, and should be removed. But no one asked me :-)

The difference becomes stark as soon as there is a method added. Once a class or interface has even a single method, the ability to pretend they are the same evaporates.

tcoz commented 7 years ago

Agreed @kylecordes, this statement is probably misusing context: "I was following angular's styleguide by using classes instead of interfaces for type definitions".

A class and an interface serve two very different purposes. One is about concrete implementation, the other abstracted behavior. I'm confident the Angular style guide would not equate the two as interchangeable.

@Snesi , the difference is, a class is a concrete thing. For example, "Car" is not a behavior, or (as Scala calls them, which to me makes a lot more sense than "interface"), a "trait". Car IS Car. But, "Car" can be "Drivable". That's a trait that can be shared across different kinds of things; a Bike, a power mower, etc. If you were to say, "Oh I just create ICar, IBike, IPowerMower"...that's misuse, which always leads to your question, "why bother with interfaces?" Better to do something like "Vehicle" (subclass Car, Bike, PowerMower), with traits IDriveable, IFuelable, etc. This way, you can even have a vehicle, say, that isn't drivable, you would create "IRemotelyOperated" or some such. ISelfDriveable extends IDriveable. You get the idea. Now you can have a Car that can be operated a variety of ways, specified by different interfaces.

But if you just create an interface that does nothing every time but mirror the entire implementation of a specific concrete thing, you'll find you end up with a lot more interfaces than you should, because there's no behavioral abstraction, and interface will just appear superfluous.

Snesi commented 7 years ago

@tcoz @kylecordes Thank you, I understand the difference in languages like Java or PHP, but if Typescript allows to use a class like if it's an interface, why not embrace that hack?

Also, you are right @tcoz. I misinterpreted the angular guideline. This is what it says about interfaces:

Do name an interface using upper camel case.

Consider naming an interface without an I prefix.

Consider using a class instead of an interface.

Why? TypeScript guidelines discourage the I prefix.

Why? A class alone is less code than a class-plus-interface.

Why? A class can act as an interface (use implements instead of extends).

Why? An interface-class can be a provider lookup token in Angular dependency injection.

It just says to consider using a class instead of an interface because it can be implemented instead of extended and because it can be a provider lookup token for dependency injection in Angular (I have no idea what that is)

tcoz commented 7 years ago

I would say for the very reason you suggest: it's a hack. I'm not sure language is a determining factor. An interface is an interface.

Now, if TypeScript has underlying technical reasons for not using interfaces the way everything else does because the language itself can't support it properly, that's a different matter. But they should make it clear that this is the case. And increasingly, it does seem interfaces can be problematic in TS.

Also the whole "without the I" thing is dubious. The C# style guide, very first example of Interface, shows:

interface IEquatable

(Note also the name implies a behavior, not a concrete "thing").

So it seems contradictory to me that TS would say "no" but C# clearly says "yes", unless the point was to be able to tell a C# interface from a TS one...which again, would be a questionable practice IMHO. Or, again, unless the reason was the underlying technology itself.

Also, say you're iterating a collection and filtering on some types. Are they interfaces, or class types? How do you know just by looking at the code? In a review (like a GitHub side-by-side), where's there's no hover tooling and so on, that could be very annoying.

Why depart from such a useful standard? Myself, that's one TS thing (if it is in fact a formal recommendation) I might discard.

paullessing commented 7 years ago

I've had a read of the styleguide, and I think it's simply poorly worded. What I believe it means is instead of doing this:

interface IItemService {}
class ItemService implements IItemService {}
class OtherService {
  constructor(itemService: IItemService) {}
}

to do this:

class ItemService {}
class OtherService {
  constructor(itemService: ItemService) {}
}

This is because in the latter case, the class itself can be injected by-reference (this is what the "provider lookup token" bit means - the class constructor is used as the thing to identify what we're trying to inject here.) The style guide is simply advising against a pattern that is quite common e.g. in Java with Spring, where every dependency should be an interface and then the DI framework figures out which implementation to use. It states that in Typescript that creates unnecessary overhead, and in 99% of cases your interface would only have one implementation anyway so it's unnecessary to prematurely optimise this by leveraging polymorphism.

I don't believe the style guide is actually stating "use classes instead of interfaces always" - I think it's stating "use just classes instead of class-plus-interface for injectable classes."

Regardless, I believe this is quite off-topic and is just making a long thread longer, so we should probably stop discussing the style guide.

thekalinga commented 7 years ago

@tcoz

Also the whole "without the I" thing is dubious. The C# style guide, very first example of Interface, shows:

interface IEquatable

(Note also the name implies a behavior, not a concrete "thing").

If you go by that logic, You don't need to pretend I to Equatable to know that its a behaviour. The able sufix of Equatable clearly implies this is a interface.

So I is completely redundant in this example.

tcoz commented 7 years ago

Some pretty easy examples where that doesn't work. Example: "IComponent", "IProvider". That would have to be "Compentable", "Providerable". That'd just be odd. Those btw are again from C#.

I really can't see a valid reason to make TS different from everything else on earth that way, particularly it's closest analog, C#.

Snesi commented 7 years ago

I'm sorry to have brought this up, but continuing this discussion about code style is completely off-topic xD.

For now, just so I don't see the warning export 'TestInterface' was not found in './test.service' I preferred to use classes instead of Interfaces, because I don't want to split every Interface into separate files.

And thanks to typescript's "hack" that allows you to use classes like if they were interfaces, whenever this bug is fixed, I could change the classes to interfaces and everything should continue working without issues.

gionkunz commented 7 years ago

Just to obscure this weird error a bit more... I have the same warning for:

// Warning: "export 'BillState' was not found in '../../state/reducers/bills'
@Input() bill: BillModel;

However, if I change the type to be an array of BillModel the error is gone.... :sheep:

// No warning :-)
@Input() bill: BillModel[];
corbfon commented 7 years ago

@gionkunz I have the same behavior as you, except that it's crashing my entire site. It's only in certain files, when I import the interface and assign it as a type, I get the error datatypes_1 is not defined. datatypes is the folder that my interface sits in. It does not matter if I import the interface from the index file in datatypes or straight from the file where it is exported. But, if I cast the object as an array, I can build the project.

slavafomin commented 7 years ago

I'm experiencing the same issue when referencing interfaces from third-party modules.

filipesilva commented 7 years ago

Heya all, I've been looking at this problem and wanted to give you an update.

There's two parts to the issue:

So for things that need to exist and be imported after compilation, like a decorated type (e.g. @Input() test: TestInterface), you will get a warning that the import cannot be found by webpack. It simply does not exist anymore after compilation (AOT does not suffer from this problem btw).

A couple of workarounds have been proposed, like keeping files that only have interfaces, or declaring the interface in the same class that uses it. These address the problem in different ways.

When you have a file composed only of interfaces, it is empty after compilation. So Webpack thinks it's a CommonJS module (instead of a ES module) and doesn't try to figure out it's exports, assuming whatever we're importing is there at runtime. See https://github.com/webpack/webpack/issues/2977#issuecomment-245898520 for more details.

Declaring the interface in the same module where you use it means there's no export to follow, so no warning.

I checked with the TypeScript folks (@mhegazy, @DanielRosenwasser) and @alexeagle, and this is indeed an unfortunate situation for which there is no clear fix.

The recommendation is to use a value that exists after compilation for decorated properties. For instance, a class or variable instead of an interface.

I know this is frustrating because you are encouraged to fully make use of TypeScript, but it is a current limitation.

I'll communicate this to docs to make sure guidance is provided.

Thanks to all that have been following this, providing context, reproductions and workarounds. I'm sorry I don't have a real fix that just makes it go away.

wilsoncook commented 7 years ago

+1

wilsoncook commented 7 years ago

@filipesilva So the best practice is to use class instead?

filipesilva commented 7 years ago

@wilsoncook yes.

mischkl commented 7 years ago

I just got this warning for the first time today, apparently because I had a file that exported a const as well as an interface. The weird thing is I had already been using the interface elsewhere (within Providers and within other models) for a while without problems. The warning didn't crop up until I tried to use the interface from a component.

The workaround was to move the exported constant out of that file. It seems as long as individual TS files contain only compile-time constructs (e.g. types, interfaces) OR runtime constructs (classes, consts), but not mixed, then the warning will not occur.

IMHO, this seems like a much better approach than telling people to switch to using classes everywhere, because while TypeScript might treat class type annotations the same way as interface type annotations, the semantics of classes in JavaScript itself are quite different. Classes in JavaScript > ES2015 mean you're expecting certain methods and you're expecting a certain prototype and a certain constructor with possibly certain static fields on it, and you're expecting to be able to "instanceof" it and always get true if it matches. To abuse classes to get type-safety on plain JS data objects is not the right way to go, IMHO.

wilsoncook commented 7 years ago

@mischkl same concern

th0r commented 7 years ago

So the best practice is to use class instead?

@filipesilva But as I understand it adds extra unused source code to the resulting bundle or I miss something?

th0r commented 7 years ago

IMHO, this seems like a much better approach than telling people to switching to using classes everywhere

@mischkl ~~The thing is you can't completely split interfaces and the real code in some situations. Here is one of them:~~

import {SomeComponent} from './some.component';

export interface SomeInterface {
  ref: ComponentRef<SomeComponent>;
}

In this case you have to import the real component to describe an interface and resulting file won't be empty after compilation.

mischkl commented 7 years ago

@th0r the example you mention should not be a problem. The problem only comes when you export a mixture of compile-time and run-time stuff in one file, e.g.:

export class SomeClass {
  foo: string;
}

export interface SomeInterface {
  bar: string;
}
th0r commented 7 years ago

@mischkl I mean your solution (extracting all compile-time stuff into a separate file) won't work for my example: I extracted SomeInterface but it depends on run-time code (SomeComponent) and resulting file won't be empty after compilation.

mischkl commented 7 years ago

@th0r I guess I didn't specify what I meant exactly enough. You can mix and match compile-time stuff with non-compile-time stuff as much as you want AFAIK, the only problems come when you export both from one file.

So the solution is to separate out all interface and type exports into extra files, which can then be imported elsewhere and used freely.

th0r commented 7 years ago

So the solution is to separate out all interface and type exports into extra files, which can then be imported elsewhere and used freely.

This won't work for my example because of this (quote from this comment):

When you have a file composed only of interfaces, it is empty after compilation. So Webpack thinks it's a CommonJS module (instead of a ES module) and doesn't try to figure out it's exports, assuming whatever we're importing is there at runtime.

In my example this file with interfaces won't be empty because you imported SomeComponent into it that is needed to describe SomeInterface.

mischkl commented 7 years ago

@th0r why don't you try it out? I think you'll find that the result is empty, despite your expectations. Keep in mind you are using the class here as a type annotation only, and more importantly you are not explicitly re-exporting it. From the outside all that is exported is the interface which consists of type annotations and nothing more.

th0r commented 7 years ago

@mischkl I'm sorry! You're right - just tried to extract them into separate files again and warnings have disappeared. Seems like I did something wrong first time 🤔

zakhenry commented 7 years ago

@filipesilva is there a typescript issue for this that can be referenced by this issue for future tracking if it gets fixed?

filipesilva commented 7 years ago

@zakhenry not that I know of, no.

reppners commented 7 years ago

Is there a reason why these warnings fail execution in the browser when its just warnings and not errors? As mentioned by someone in cli@1.0.0 it seemed no showstopper for development but as of cli@1.0.1 it halts development completely when it happens. My team is in the progress of migrating an app with 150+ components and suddenly this issue appeared for us. I'm still goofing around trying to find workarounds. It seems to have appeared completely random after having migrated roughly 30 components.. EDIT: Upgrading first helps.. had a specific version entered in package.json facepalm

So far I've found that any kind of union type on affected decorated properties will remove the warning, e.g.

@Input() someProp:MyMagicalInterface

will result in a warning while

@Input() someProp:MyMagicalInterface | undefined

will work. And to get completely funky

@Input() someProp:MyMagicalInterface | MyMagicalInterface

does work also. ¯_(ツ)_/¯

axtho commented 7 years ago

for whatever it's worth ... when you change the module in the tsconfig to "commonjs" the error goes away. This may not work for most situations, in mine it does because I am using a core module that is compiled outside of the cli project (with ngc) and later linked into it.

cipchk commented 7 years ago
// ./src/app/wx.d.ts
export declare function config(): void;
// ./src/app/app.component.ts
import * as wx from './wx.d';

// using
wx.config();

ng serve throw Module build failed: Error: Debug Failure. False expression: Output generation failed.

strake7 commented 7 years ago

Changing tsconfig module to "commonjs" fixed the warnings for me. Previously it was set to "es2015"

remmeier commented 7 years ago

is there a way to just silence that warning?

zakhenry commented 7 years ago

@remmeier see the comment a few above - you can suppress the warning simply by making the type a union of itself. The same type safety is achieved, and the warning goes away. It is by no means a fix, but at least you can continue developing

RenaldasK commented 7 years ago

The issue still exists, why is this closed?

tytskyi commented 7 years ago

@RenaldasK there is long explanation (shortly - it is the limitation which currently cannot be solved) https://github.com/angular/angular-cli/issues/2034#issuecomment-302666897

Try to rewrite interfaces to classes if possible (this works for me). Or one of the proposed solutions above (i did not try any of them).

MattMorrisDev commented 7 years ago

Aside from the console warnings, is there any actual reason to avoid this issue?

RenaldasK commented 7 years ago

All of the solutions above feel like a hacks, and putting a single line interface in its own file seems like a waste of time...

bickycheese commented 7 years ago

Having the same issue with a scenario like this:

export class Something { ... }
export type SomethingKey = Pick<Something , 'keyPropertyName'>;

in one file. However when I add another interface, class or type to that file, the interface is resolved perfectly fine. This seem to be only the issue with the Pick<..> type. It works, compiles, transpiles, it only gives a warning in the CLI...

mmc41 commented 7 years ago

@filipesilva Thanks for looking into the problem in detail. Easiest workaround I have found is to declare a local alias type in the component file for the imported type when needed in an @Input(). So for your example, rather than:

import {TestInterface} from ...
...
@Input() test: TestInterface

do a

import {TestInterface} from ...
type TestInterfaceAliasHack = TestInterface;
...
@Input() test: TestInterfaceAliasHack
Ks89 commented 7 years ago

This is really annoying. Is there a real fix and not simply temporary workarounds?

elvisbegovic commented 7 years ago

This issue's very Annoying !

remmeier commented 7 years ago

For the time being just a flag to silence the warnings would be great as everything works perfectly fine regardless of the warnings.

ndcunningham commented 7 years ago

They said working on it, i have the same issue just have to wait or work with a work-around if possible.