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

Why styleguide suggest use class instead of interface? #19632

Closed
copiali opened this issue Oct 10, 2017 · 20 comments
Closed

Why styleguide suggest use class instead of interface? #19632

copiali opened this issue Oct 10, 2017 · 20 comments
Assignees
Labels
area: core Issues related to the framework runtime freq3: high type: bug/fix
Milestone

Comments

@copiali
Copy link

copiali commented Oct 10, 2017

In style guid, it says consider use class instead of interface
https://angular.io/guide/styleguide#interfaces

Also in the example file hero.model.ts it's using class:
export class Hero {
id: number;
name: string;
}

I think in this case better use interface instead of class? As typescript best practice is that in this case. Unlike classes, interfaces are completely removed during compilation and so they will not add any unnecessary bloat to our final JavaScript code.

Any reason to use class instead of interface in this case?? Or it's better to use interface in this case?

@about-code
Copy link

about-code commented Oct 10, 2017

I can't comment on the reasoning of the styleguide authors, but I can provide some thoughts on why I find classes in many situations more suitable than interfaces and why I think the styleguide-advice is justified. The write-up became much longer than I intended, but hope you appreciate the reading.

First you should note, that the styleguide-example gives an example which is related to dependency injection, so I'll refer to DI at first.

  • In languages with a runtime equivalent of interfaces, like e.g. Java, it's a common and recommended practice to define an interface type via an interface keyword and refer to it on the injection-target side. That is, of course, to prevent the injection-target from becoming dependent on a concrete implementation of an interface. Like you mentioned, in TypeScript though, interfaces have no runtime equivalent and can't be used by Angular to look up an interface implementation type. Instead Angular's DI is based on a mapping of an object reference A to some other object reference B where A for example is a function object (which you'll get at runtime when importing a class but not when importing an interface). So the closest equivalent to interface-based DI of Java is class-based DI in TypeScript. In terms of type-theory interfaces are actually only purely abstract classes, anyway (anybody correct me, if I am wrong, here). The bottom-line of all this is: as soon as you use a class, you get a referencable function which you can use as a DI-Token, so: using classes instead of interfaces is the right advice, here.
  • For model types I have found classes also more beneficial than interfaces. With interfaces, for example, you can create semi-instances of that interface type all over your code using simple object-literal notation (var instance: FooInterface = { ... };). In fact they are all completely distinct instances and you don't have a real link between type-level and instance-level at runtime which also comes at a price:
    • you loose the ability to control instantiation at a single place in code, meaning there's no central constructor hook where you can place constructor or initialization logic* should you need it if your code evolves or requirements change (I know YAGNI, btw)
    • you can't declare private properties in an interface
    • object-literal-instances are completely distinct objects at runtime sharing only the Object prototype. Introducing Getter/Setter-logic common to all instances of a type, later, e.g. via Object.defineProperty isn't possible, either. Once I've joined a project where we had object literals working with an ISO-date-string all over the code. Then later we found, that it would have been better to guarantee that each instance holds a Date object internally. Because of all the instances having been completely distinct objects, there was no other way than to modify every statement where such instances have been created or its date property had been written. This was even more a problem since we hadn't TypeScript at the time but only JavaScript and we felt that we lost oversight where and how instances were created or how their date property was accessed (have I said, that I know YAGNI, btw?). TypeScript would have helped us a lot, given noImplicitAny: true. However, our lesson learned was: we essentially missed well-defined constructor and setter logic and didn't have consistent object shapes and should have used classes/constructor functions instead of object-literals (interfaces) in the first place.

Based on that experience, I consider TypeScript interfaces and object-literals a good fit for parameter objects and actual value types. They are less beneficial for model types or app-global application state, IMHO.

I am even used to pass JSON-deserialized plain objects into a proper class constructor before using them to get a proper prototype chain and well-defined instances (it seems like instantiating an instance twice but see here that it is still faster than setting the prototype manually on an already existing instance) . Yet I wouldn't dare to say its the way to go for everyone and sometimes exceptions to the rule still make sense.

Nevertheless, I don't share general criticism of classes vs. object-literals in terms of "performance". Unless you need to create a real mass of tens of thousands of objects at once, speed is seldom a real matter, FWIW. At least in client-side JavaScript, even if you have grids with thousands of potential items you still almost always want to implement some kind of lazy loading for the reason of load-times, network latency and scalability, limited display size etc. not due to JS speed. So you often load and instantiate a batch of a few hundred items at once where performance issues aren't significant. If your constructor initializes all class properties at instantiation time, then some JS engines can apply hidden class optimization to optimize speed of object creation. In terms of memory-efficiency, classes are superior as soon as your model needs methods.

So given all that, I think its fair to propose classes as a best-practice in a styleguide.


* You could mitigate this with a factory pattern + interfaces but

  1. there's no guarantee that the factory is always used to create an instance. Its not enforced by the actual type definition
  2. factories would just add bloat

@changLiuUNSW
Copy link

Sometimes we only need the definition for the server data, some models. The interface can perfectly serve this purpose without introducing additional overhead for the final output, because interfaces are completely removed during compilation.
In my opinions, we should choose the one (interface or class) based on different requirements.

@about-code
Copy link

In my opinions, we should choose the one (interface or class) based on different requirements.

Choosing what best fits requirements is always the best thing to do. The term consider in the styleguide implies exactly that.

@Toxicable
Copy link

Toxicable commented Oct 12, 2017

For me I have 2 situations where using interfaces + objects works much better than using classes, because of this we have no reason to use classes (for object models) in our apps.

Universal State Transfer - Classes can be serialised into objects but they cannot be automatically instantiated from json back into the class, therefore it requires more work from the dev aswell as more code, when using object + interface requires 0 extra work and 0 runtime overhead.

Firebase - For very similar reasons as above you cannot use classes with firebase, infact if you try use a class firebase will throw.

@Splaktar
Copy link
Member

Splaktar commented Dec 7, 2017

My questions were the following:

Interesting that the official Style Guide recommends using Classes over Interfaces (https://angular.io/guide/styleguide#interfaces). It doesn’t really say when, so it appears as if ‘always’ is the recommendation.

Also interesting that the Classes section is so bare (https://angular.io/guide/styleguide#classes).

I’m seeing some of this approach lately and I find it harder to read:

export class MyGraph {
  constructor(public name: string, public visits: number, public change: number, public denials: number, public date: Date,
                      public allowed: boolean, public weighted: boolean) {}
}

Compared to:

export interface MyGraph {
  public name: string;
  public visits: number;
  public change: number;
  public denials: number;
  public allowed: boolean;
  public weighted: boolean;
}

Am I off base somehow in recommending to use interface for data model objects that contain no logic and will never be instantiated?

Outcome

Just discussed this with @wardbell. He said that I should mention @kapunahelewong and suggest that this recommendation be clarified. His quote was "Maybe we weren’t clear. Classes for components, directives, pipes, and services. Because Angular and DI insist. Otherwise, the guide does not care."

The current recommendations seem to indicate that using interface for data models and other use cases is incorrect. The clarification should make it clear that this guidance is only for "services and declarables". I'm not sure that's that best way to describe it, but there is some need for clarification here.

@johnpapa
Copy link
Contributor

johnpapa commented Dec 7, 2017

This can be removed or it can stay.

The guide clearly calls this out as a consider. Which means

Consider guidelines should generally be followed. If you fully understand the meaning behind the guideline and have a good reason to deviate, then do so. Please strive to be consistent.

So if you disagree, then do not follow that. It's OK. The key is to consider your choice and be consistent.

@copiali
Copy link
Author

copiali commented Dec 7, 2017

Yeah totally understand. The thing is when we review the pull request, we normally add style guide as the reference to suggest them follow best practice. But the thing I found is even I give the reason why better to use interface in this case, but they might say the style guide use class in this case instead of interface. Even they don't know why but they think there must be some reasons style guide do this. So they will insist to use class instead of interface. That's why I ask this question whether there's some reason to use class instead of interface in this case.

@johnpapa
Copy link
Contributor

johnpapa commented Dec 7, 2017

Have them read the first paragraph (what I quoted above). "Consider" or more clearly "think about it and have a discussion with your team".

We'll never get that type of wording perfect.

But ... PRs are always welcome :)

@kapunahelewong
Copy link
Contributor

Hey @jenniferfell, this issue is part way addressed in that the style guide is updated. We still need to assess the example as in #21186 and see if it needs to be refactored. You could probably close this issue but it will serve as a good reference for one of the writers.

@ngbot ngbot bot added this to the needsTriage milestone Feb 26, 2018
@ngbot ngbot bot modified the milestones: needsTriage, Backlog May 3, 2018
@jenniferfell
Copy link
Contributor

Closing because the style guide is correct. Keeping #21186 open to consider the implementation of the TOH tutorial. Thanks, everyone.

@atodd-geoplan
Copy link

@about-code of course you can now inject interfaces with an InjectionToken:

export const AuthenticationProvider = new InjectionToken(
  "AuthenticationProvider",
  { providedIn: "root", factory: () => new CognitoAuthenticationProvider() }
);

...

@Injectable()
export class CognitoAuthenticationProvider implements IAuthenticationProvider {

...

@Injectable({
  providedIn: "root"
})
export class AuthenticationService {
  constructor(
    @Inject(AuthenticationProvider)
    private authenticationProvider: IAuthenticationProvider,
    private http: HttpClient
  ) {}

@about-code
Copy link

about-code commented Aug 23, 2018

@atodd-geoplan I would probably still prefer purely abstract classes over interfaces with DI - at least and in particular with NgModule providers or Component providers. I would also most likely choose an abstract class with tree-shakable providers, however I do see that your approach does provide a nice solution to something which bugs me at times, with tree-shakable providers, that is, the need to make an interface type depend on a particular default implementation (see below).

1. But let's assume we are not using tree-shakable providers, which was the premise in my comment above. Why would I continue to choose purely abstract classes over interfaces:

When transpiling a purely abstract class to JavaScript, where the abstract class has only abstract methods and only uninitialized properties, what remains after transpilation is an exported variable pointing to an empty constructor function. This empty function is actually equal to the token constant you are exporting in your example.

The advantage with the purely abstract class, though:

  • you don't need to create a separate Injection Token
  • you don't need the @Inject() parameter decorator in your injection target (AuthenticationService) anymore, because Angular can derive the token from the type metadata
  • you can use the class as a runtime token and as a design-time type

Example with NgModule decorator:

@Injectable()
export abstract class IAuthenticationProvider {
  // your interface as purely abstract class (think of it as an interface class);
  // when transpiled to ES5 (with ES2015 modules), it should end up with something like
  //   `export const IAuthenticationProvider = function() {};`
  // compare this to
  //   `export const AuthenticationProvider = new InjectionToken() {};` 
  // in your examples above;
}

// Implementation implements the abstract class 
export class CognitoAuthenticationProvider implements IAuthenticationProvider {}

// We don't need @Inject() because Angular can infer the token from the type metadata
@Injectable()
export class AuthenticationService {
  constructor(
    private authenticationProvider: IAuthenticationProvider,
    private http: HttpClient
  ) {}
}

// mapping the interface class to an implementation, with an NgModule provider in this case
@NgModule({
   providers: [
       { provide: IAuthenticationProvider, useFactory: () => new CognitoAuthenticationProvider() }
   ]
})
export class AppModule {}

2. Tree-shakable providers

Your example is still interesting with respect to tree-shakable providers. Here we often have the situation that an interface type has to be a default implementation or has to create a dependeny on such a concrete implementation via the @Injectable decorator and a useClass or useFactory declaration, for example:

@Injectable({
   providedIn: 'root',
   useFactory: () => new CognitoAuthenticationProvider()
})
export abstract class IAuthenticationProvider {}

Requiring an interface type to know and depend on a particular implementation looks a bit strange if you know how DI and interfaces work in other languages, like Java. It's not a severe problem, because you can still choose to override a default implementation with an NgModules provider. Nevertheless it would be desirable if one could use @Injectable on the implementing side as well and make the implementation declare what it implements, so something like

@Injectable({ providedIn: 'root' })
export abstract class IAuthenticationProvider {}

@Injectable({ provideAs: IAuthenticationProvider })
export abstract class CognitoAuthenticationProvider  implements IAuthenticationProvider {}

Unfortunately this isn't possible, though. Therefore I find your example interesting in that it shows how to decouple an interface from a particular implementation when using tree-shakable providers as of today (see here for another example of what I mean).

@atodd-geoplan
Copy link

@about-code thanks for taking the time to show another way to do this. Both approaches are very similar.

I have an inbuilt tendency to try and use the language constructs as they were meant to be - i never really think about compilation, or tree-shaking, i just want my code representative. I also come from a .Net background, where I heavily use interfaces for DI. So it is a habit thing more than anything. So I would probably stick with an interface and the InjectionToken. Not disregarding your approach it is absolutely fine.

@about-code
Copy link

Not disregarding your approach it is absolutely fine.

Same goes for me. Thank you for your ideas.

@about-code
Copy link

@atodd-geoplan Just let me add: it should be possible for you to omit the @Injectable() decorator on CognitoAuthenticationProvider and thus make your provider interface and its implementation less dependent on the Angular framework (DI-wise). The reason is: your implementation type doesn't act as an injection token and you're controlling its instantiation yourself with a factory. So the decorator shouldn't be necessary to make things work.

@rhyous
Copy link

rhyous commented Aug 24, 2018

Another interesting thought is web assembly. Sure, it might seem like a good idea to not use Interface-based design, because we are compiling to javascript, and javascript doesn't have interfaces. I foresee that TypeScript will start compiling directly to web assembly and bypass javascript altogether. At that point, does it make sense to make a critical architectural decision based on a short term compilation target? Yagni probably says so, but an Api developer might want to consider interface-based design as a long term solution. Of course, when has anyone in the web/js world thought long term ;-).

@about-code
Copy link

about-code commented Aug 24, 2018

@rhyous So far any requests related to TS->WASM compilation were marked out of scope by the TypeScript team. There are only independent third-party experiments attempting to compile TS to WASM but they mostly use only a subset of TypeScript. WebAssembly isn't meant to replace JavaScript. So JavaScript is certainly not a "short-term compilation target" for TypeScript. Making architectural decisions based on such a vague assumption is critical, either.

Using classes, abstract or non-abstract, is not really going against the principles of interface-based design. In fact you can use any class right-hand side of the implements clause as well (see Section "Classes as Interfaces" in the TypeScript Handbook).

@gbpatil
Copy link

gbpatil commented Oct 24, 2018

Angular styleguide now recommends Interfaces for data models. It seems it has been updated later.
https://angular.io/guide/styleguide#style-03-03

@Dylan-Israel
Copy link

Dylan-Israel commented Jun 11, 2019

I try to avoid interfaces whenever possible in favor of classes. The main reason is I can give default values and it prevents a lot of scared / defensive coding especially when I can set an empty array for a list. You also never know when you may need to add a get or method down the road. Furthermore, when you test things it makes setup go much quicker and most of the time people create an object manually instead of just using a constructor in the code anyhow. It just feels like it provides more value to me.

Example below:

export class Example Class {
    public readonly _id: string;
    public cartName: string;
    public list: Example2[];

    constructor(data?: any) {
        const defaults = {
            cartName: '',
            list: [],
            ...data
        };

        this._id = defaults._id;
        this.cartName = defaults.cartName;
        this.list = defaults.list.map((example) => new Example2(example));
    }
}

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime freq3: high type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.