DEV Community

loading...
Cover image for 7 Deadly Sins of Angular
This is Angular

7 Deadly Sins of Angular

armandotrue profile image Armen Vardanyan ・7 min read

Original cover photo by Nick Gavrilov on Unsplash.

Angular is known to be an opinionated and prescriptive framework. Despite that, it has footguns like every other technology. In this list, we review the most common and deadly sins that are committed in Angular applications. You will learn how to make amends to redeem your eternal Angular soul from damnation.

For this article, we created a rating system that categorizes each Angular sin based on the severity of its impact and the precise influence it has on an Angular codebase. We value the sins based on how they affect:

  • Potential for bugs
  • Maintainability
  • Architecture
  • Performance
  • Scalability
  • Bundle size
  • Accessibility
  • Code reuse

#7: Eagerly loading all features

Not utilizing lazy loading in our applications is a colossal sin, particularly because lazy loading

  • Is simple
  • Is built-in
  • Provides very tangible performance and network usage improvements

In short, use lazy loading where applicable by carefully dividing your application into logically sound modules that incorporate related logic, then load those modules lazily.

Amends: Either use the Angular Router's lazy loading feature or use the function-like dynamic import statement.

#6: Grouping classes by type

We have often seen codebase with folders called services, pipes, directives, and components inside an Angular application. On the surface, this might look reasonable: After all, if I am looking for some service, it makes sense to look for it under a folder named services. But in reality, this poses several problems:

  • The type grouping folders end up as junk drawers of unrelated classes that are hard to navigate.
  • Working on a component that uses the service also requires navigating to a very distant folder. This is a violation of the Principle of Proximity which states that files that often change at the same time should be located closely.
  • Makes our applications less scalable: If all our services, directives, pipes, and components are dumped in the same directories, it means more refactoring.

So how do we solve this? Here are some tips:

  • Group by feature first, then by layer, then finally maybe by type.
  • If a service is relevant to an Angular module, place it inside that module first.
  • Maybe create a submodule if the module is large enough.
  • Then the most basic module can have a services folder which contains services only relevant to that module.

A relevant example is an admin module that contains submoldules that allow the user to manage companies and users that are associated with them. It is natural to create a "users" module and a "companies" module, and provide the "UserService" and the "CompanyService" in the respective modules. But imagine now we need to display a dropdown with company names in the user detail page, so we can add that user as an employee to some company. Obviously we have to use the "CompanyService", but it is inside the "CompanyModule". So what we need is move it up into the "AdminModule", so both modules can have access to it. We will then do similar refactorings in all such relevant scenarios.

Here is a nice folder structure that resembles a good approach to frontend architecture from this example:

├───app
│ │ app-routing.module.ts
│ │ app.component.ts
│ │ app.module.ts
│ │
│ ├───admin
│ │ │ admin.component.ts
│ │ │ admin.module.ts
│ │ │ admin.routing.ts
│ │ │
│ │ ├───companies
│ │ │ companies.component.ts
│ │ │ companies.module.ts
│ │ │ companies.routing.ts
│ │ │
│ │ │───services
│ │ │ companies.service.ts
│ │ │
│ │ └───users
│ │ │ users.component.ts
│ │ │ users.module.ts
│ │ │ users.routing.ts
│ │
│ │───services
│ │ users.service.ts
│ │
│ └───common
│ │ common.module.ts
│ │
│ ├───directives
│ │ error-highlight.directive.ts
│ │
│ ├───pipes
│ │ includes.pipe.ts
│ │
│ └───services
│ local-storage.service.ts
Enter fullscreen mode Exit fullscreen mode

You can find the example app here.

#5: Subscribing manually to an observable

In Essence, subscribing to an Observable manually means performing imperative logic. Why would anyone subscribe to an Observable manually anyway? If it is not to perform an imperative action, then it is useless. If we can express the same thing using RxJS operators in a more declarative way, then there is no need to subscribe to an Observable; we could just use the AsyncPipe. However, notice that the AsyncPipe does not handle errors and completions Rule of thumb: Only subscribe to an Observable manually if you need to perform an imperative operation that cannot be done in another way. A very common example of that is enabling/disabling a FormControl depending on the latest emission from an RxJS stream. It can only be done using FormControl's enable/disable methods, which are imperative by themselves, thus the need to subscribe.

#4: Big, hairy components

Imagine a whole Angular application in one component. Are you laughing? We've seen this. The same reasons for this being a deadly sin, applies to components at a smaller scale as well. Do you have one component per feature or per page? You're doing it wrong!

With an entire feature in just a single component, you're giving Angular a hard time keeping performance high since every change causes all data bindings to be re-evaluated and dirty-checked. What's worse, you leave this unmaintainable mess for your co-workers or your future self.

There are several reasons why a component can grow too big. It can be dealing with too many responsibilities. Ideally, components should be thin wrappers gluing user interactions and application events together with the UI.

So in essence, there are things that our components should and should not do. Here are some things a component should do :

  • Work with the DOM
  • Display data from store/services
  • Handle its lifecycle events
  • Manage forms (template-driven/reactive)
  • User interactions
  • Pass data to child components

Things a component should not do:

  • Directly load data
  • Modify global state
  • Work with storages directly (cookies, localStorage, etc)
  • Work with real time connections directly (WebSockets and more)
  • Handle custom DOM-related scenarios (for example, highlighting invalid inputs). Those can be extracted to services to be more reusable.

Variation: Big, hairy services

  • Sometimes we fail to organize our services correctly.
  • Usually, services dealing with external data (loaded by HTTP, for example) are to be sorted by feature.
  • But sometimes logic gets mixed. For example, a service called ArticleService might start making HTTP requests that create/update bookmarks or tags. That is a clear violation of the Single Responsibility principle. Good examples of what an ArticleService should do are adding an article to a database, deleting it, getting/sorting/filtering a list of many articles, essentially, CRUD (create, read, update, delete).
  • To avoid situations like this, always categorize your services based on which data features they work with, and don't mix them with services that provide abstraction layers, for example an adapter for a third-party library.

#3: Putting complex logic in component templates

While declarative component templates are nice, they shouldn't be used for complex logic, presentational or otherwise. Strict template type-checking removes silly mistakes such as type errors or typos.

Placing logic in component templates forces you to test it through the DOM. Component tests are slower than unit tests because the component template needs to be compiled and a lot of setup happens. Additionally, logic placed in component templates cannot be reused.

At the very least, extract logic from a component template into the component model.

However, you're better off extracting all forms of logic into services. Presentational logic belongs in a presenter. Non-presentational logic belongs in other service types. Read #4: Big, hairy components for more on this topic.

#2: Putting all declarations in AppModule

Frankly speaking, modules are probably the most heavily criticized feature of Angular. They are hard to explain to newcomers, sometimes difficult to maintain and an overall source for confusion. So one really bad idea would be to put all of our imports/exports/declarations directly into our root AppModule. This not only violates the principle of separation of concerns, but also makes the AppModule insanely bloated the more complex our application gets. But thankfully, there is a relatively easy solution to this

  1. Create Feature modules and separate different feature component declarations into them
  2. For components/pipes/directives/services used by different modules create a Shared Module

But the second bullet point can also become a bit sinful if we start

Variation: Putting too many declarations in SharedModule

To avoid this, we might start grouping dependencies inside Feature modules too. For example, if we have an AdminModule, which contains UserModule and AccountModule, and both those modules use a service called ManagementService, we can move that service to be inside AdminModule rather than the entire application module; this way, Feature Modules can have their own Shared Modules

#1: Using imperative programming and default change detection

Some sins are understandable. Despite being built around RxJS, Angular itself still encourages imperative programming: the state is an object that we can freely modify as we see fit, and Angular Change Detection will update the DOM accordingly. But there are multiple problems with this approach:

  • Imperative programming is too verbose and hard to understand; very often one would have to read an entire passage of code to get the idea how a state of data is modified
  • Imperative programming is built around mutating state: an object under the same reference gets mutated all the time, which can become a constant source of weird bugs: your state has changed, but you have no idea how and from where!
  • Default Angular Change Detection is more or less efficient, but it still makes lots of unnecessary steps, which we can easily skip

There are several ways to redeem this particular sin:

  • Most importantly, ditch imperative programming in favour of declarative, use the best practices of functional programming, write pure functions, be very explicit, use composition, avoid bad practices
  • Use more and more RxJS Observables, operators, and start describing your states and its mutations as streams
  • Stop mutating data manually, switch to ChangeDetectionStrategy.OnPush, use Observables together with the async pipe
  • Also consider using a State Management System like NGRX

Conclusion

There are lots of things that can go wrong when developing a frontend application; this guide was meant to showcase the most common and important things developers tend to do in bad ways when using Angular. Hopefully, when you review your applications and remove some of the sins that might be present there, you will end up with a more scalable, understandable and manageable codebase

Discussion (28)

pic
Editor guide
Collapse
anfibiacreativa profile image
Natalia Venditto

Great post! I agree with everything except point #6.

I am a big fan of organizing per type of class and works really well for me, especially in terms of code reusability. Sure, there may be a specific service bound to a specific component, or a pipe you only use in one component...but, in general, I try think very generic and favor reusability.

Collapse
layzee profile image
Collapse
anfibiacreativa profile image
Natalia Venditto

Well, I don't consider it sin, Lars. :)

Thread Thread
layzee profile image
Lars Gyrup Brink Nielsen • Edited

It's the deadliest sin of them all if you ask me 😃

Thread Thread
anfibiacreativa profile image
Natalia Venditto • Edited

To each their own. ;) Or perhaps we are talking about different use cases, because I can't believe you violate DRY or create cross component dependencies, which is effectively the deadliest sin I can think of.

Thread Thread
layzee profile image
Lars Gyrup Brink Nielsen • Edited

In an application, I would insist on group by feature:

src/
- app/
  - admin/
    - admin-dashboard.component.ts
    - manage-crises.component.ts
  - auth/
    - auth.service.ts
    - login.component.ts
  - crisis-center/
    - crisis.service.ts
    - crisis-center.component.ts
  - heroes/
    - hero.service.ts
    - hero-list.component.ts
Enter fullscreen mode Exit fullscreen mode

not group by type:

src/
- app/
  - components/
    - admin-dashboard.component.ts
    - crisis-center.component.ts
    - hero-list.component.ts
    - login.component.ts
    - manage-crises.component.ts
  - services/
    - auth.service.ts
    - crisis.service.ts
    - hero.service.ts
Enter fullscreen mode Exit fullscreen mode

Grouping by type quickly gets out of hand and violates the Principle of Proximity. It doesn't scale well and lowers maintainability.

Thread Thread
anfibiacreativa profile image
Natalia Venditto

I am not really with energy enough to have a very long discussion on this, and appreciate your opinion, but do not share it at a 100%. Usually all services inherit (or should, from a base) and that base needs a common place to be. Some services are shared between several domains, and they also need a common place to be at. Creating cross domain dependencies is a far worse issue. The principle of proximity admits exceptions. This scenario is valid for a small to medium sized application. For an enterprise application, rewriting the same code snippets over and over to maintain them in proximity, will cause you a real maintenance issue.

Also my impression is that people need to get better at browsing with their IDE's. And I mean by class and by code inside a folder.

We can't always agree, Lars. :D We do on other items though!

Thread Thread
layzee profile image
Lars Gyrup Brink Nielsen • Edited

Interesting, I rarely find the need to use inheritance. I favor composition over inheritance. It's got a much looser coupling.

Classes that are shared between feature or domains can live in libraries or folders inside of a shared grouping folder.

About DRY, I think it's generally being used too much as an excuse without considering it's trade-offs. An abstraction made too early can become very expensive and difficult to undo later in a project's life span.

Thread Thread
anfibiacreativa profile image
Natalia Venditto

"Classes that are shared between feature or domains can live in libraries or folders inside of a shared grouping folder."

That was my point.

Collapse
raysuelzer profile image
Ray Suelzer

"Subscribing manually to an observable"
I somewhat disagree here. The async pipe will create a new subscription for each occurrence. In all but all but basic applications this is not ideal, you can end up with dozens of subscriptions to the same Observable, when if you subscribe manually and and set the property in the component, you will only need one. I've had massive performance increases on pages by being extremely careful about using the async pipe.
Additionally, as you pointed out, the async pipe does not handle errors. I have also run into very hard debug issues regarding component lifecycle and initialization of Observables when using the async pipe. I'd actually say avoid async pipes unless it's a really basic page or application. Instead, create a mixin which contains a disposer$ Subject that is called via ngOnDestroy and use the takeUntil(this.disposer$) on your observables in components. It's a clean pattern that avoids the performance hits and lost of functionality that come with the async pipe.

Example WithDisposableSubjectMixin

import { OnDestroy } from '@angular/core';
import { Constructor } from '@angular/material/core/common-behaviors/constructor';
import { Subject } from 'rxjs';

export function WithDisposableSubject<T extends Constructor<{}>>(Base: T = (class { } as any)) {
  return class extends Base implements OnDestroy {
    disposer$ = new Subject<any>();

    ngOnDestroy() {
      this.disposer$.next();
      this.disposer$.complete();
    }
  };
}
Enter fullscreen mode Exit fullscreen mode

Then use in component

@Component({
  selector: 'app-entity-info-page',
  templateUrl: './entity-info.page.html',
})
export class EntityInfoPage extends
  WithDisposableSubject() implements OnInit {

   ngOnInit() {
     yourObservable$.pipe(takeUntil(this.disposer$))
   }
}
Enter fullscreen mode Exit fullscreen mode
Collapse
layzee profile image
Lars Gyrup Brink Nielsen

Hi Ray,

Thank you for your feedback. Personally, I use container components and presentational components. This solves the issue with multiple subscriptions since the container component subscribes to the observable exactly once. It also solves the issue you mention with observable subscription time since the presentational component is not aware of observable values.

Collapse
layzee profile image
Lars Gyrup Brink Nielsen

Your solution comes with a footgun. If the takeUntil operation is placed in the wrong position of the observable pipeline, it will lead to unforeseen consequences such as memory leaks and duplicate side effects.

Collapse
raysuelzer profile image
Ray Suelzer

Not to mention, using an async pipe inside of an *ngIf can end up causing a massive number of subscriptions to be created an disposed if an element is toggled frequently.

<strong *ngIf="visible">{{totalCount$ | async}} Selected</strong>
Every time that visible switches from true to false, a brand new subscription is created/disposed to $totalCount. This can have serious performance impacts on an application, not mention it's very easy to end up with this type of code triggering a ton of network requests if totalCount$ is hitting the server.

I'm sure someone has written up a good article on the dangers of the async pipe.

Collapse
layzee profile image
Lars Gyrup Brink Nielsen • Edited

I'm sure someone has written up a good article on the dangers of the async pipe.

I think AsyncPipe is grossly over-rated. Michael Hladky has talked a lot about what AsyncPipe is missing. He's even working on a range of replacements in the RxAngular (@rx-angular/*) packages.

Come to think of it, I interviewed him about this very topic.

Collapse
lifelongthinker profile image
Sebastian

Nice post, concise and clear explanations. Kudos to you, Armen and Lars.

For beginners, I believe a few more source snippets and examples might be handy.

Great work!

Collapse
gsarciotto profile image
Giovanni Sarciotto

Great article. I think that 6 is often overlooked, currently I am working on a legacy project with this file structure and it is impossible to find anything, I always need to do grep in the src folder to find a component.

Collapse
layzee profile image
Lars Gyrup Brink Nielsen

Thank you for your feedback, Giovanni 🙇‍♂️

Personally, I think this is a major issue that too few teams and organizations deal with. Even with tooling, it's hard to get right. Nx CLI gives us most of the tooling we need, but it takes time to learn how to approach this concept.

Collapse
ky1e_s profile image
Kyle Stephens

Imperative programming is too verbose and hard to understand; very often one would have to read an entire passage of code to get the idea how a state of data is modified

I'm not sure it's that black and white. I've seen both functional programming that's difficult to reason about and imperative programming that's concise and easy to understand.

Collapse
mfp22 profile image
Mike Pearson

Great article! I just barely posted an article explaining how to get around this:
"Only subscribe to an Observable manually if you need to perform an imperative operation that cannot be done in another way. A very common example of that is enabling/disabling a FormControl depending on the latest emission from an RxJS stream."

Collapse
layzee profile image
Collapse
eekee profile image
Ethan Gardener

I know nothing about Angular, but was interested to see the Principle of Proximity listed. Unix violates this very heavily which was toxic to me when trying to learn Linux with poor documentation in the 90s. Unix has reasons which argued forcefully by many, but it's still toxic for clueless people like me. :) There is one arguably good reason in there: it's so partitions other than /var can be mounted read-only, but there is still a lot of other separation with varying strength of argument behind it. IMO, it all comes down to failure to separate implemetation from interface. A very simple registry would make more sense than "bin" directories. (And what kind of a name is "bin" anyway? "Binaries" was already a catch-all term including image files in the 90s.)

Collapse
swisscoder profile image
Steve

Thanks a lot for this interesting article, hope I don't come across unfriendly or rude :D

While I agree that it's not ideal to simply group stuff by type. I find this example structure is flawed.

  • Every Component hat its own Module? why?
  • Since one paragraph earlier lazy loading is mentioned. you have split logic into modules in 2 different ways: --app -- --admin -- -- --companies -- -- --users -- --common

In one case you have the common module, which is probably used by admin/companies/users but is outside in a different folder.
And then there are the companies and users module which are contained inside the admin modules structure but both depend on the admin module.

If you would apply the same logic in both cases you would either get:
--app
-- --common
-- -- --admin
-- -- -- --companies
-- -- -- --users
or:
--app
-- --admin
-- --companies
-- --users
-- --common

I personally think, you should heed your own sound advice:
"2. For components/pipes/directives/services used by different modules create a Shared Module"
There is no reason to have only 1 shared module that's named "shared" or "common", simply create a module, whenever you reuse logic/components/services in several (other) modules. so this module structure would be fine:
--app
-- --admin
-- --companies
-- --users
-- --common
and no, not use that Variation of yours.
"For example, if we have an AdminModule, which contains UserModule and AccountModule"
=> modules containing modules is not the correct understanding of modules I think.
Modules contain components and modules reference other modules, but modules shouldn't contain modules.

the following structure would be fine if companies and users would be components only, but not if they are modules on their own:
--admin
-- --companies
-- --users

Collapse
kalashin1 profile image
Kinanee Samson

I am guilty of;

  • Not utilizing lazy loading
  • Consuming Observables without async pipe, and I'm not doing any extra stuff.
  • Grouping files by type
Collapse
layzee profile image
Lars Gyrup Brink Nielsen • Edited

10 component refactors, -1s critical load time, restructure your files and you are redeemed.

God forgives you. Now forgive yourself.

Collapse
simbiosis profile image
SIMBIOSIS

Great article still when I am not very versed on Angular. But still can comprehend the logic behind each stated sin.
Thank you very much for the tips.

Collapse
gusgonnet profile image
Gustavo

What's worse, you leave this unmaintainable mess for your co-workers or your future self.

spot on! Great article, I learned a lot from it.
Thank you

Collapse
oscarlagatta profile image
Oscar Lagatta

Well done you both 👍 very informative and useful. Great job 👏

Collapse
layzee profile image
Lars Gyrup Brink Nielsen

Thank you, Oscar 🙂