DEV Community

7 Deadly Sins of Angular

Armen Vardanyan on March 23, 2021

Original cover photo by Nick Gavrilov on Unsplash. Angular is known to be an opinionated and prescriptive framework. Despite that, it has footguns...

Read full post

Natalia Venditto • Mar 26 '21

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.

Lars Gyrup Brink Nielsen • Mar 30 '21

Natalia Venditto • Apr 3 '21

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

Lars Gyrup Brink Nielsen • Apr 3 '21 • Edited

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

Natalia Venditto • Apr 3 '21 • 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.

Lars Gyrup Brink Nielsen • Apr 4 '21 • 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

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

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

Natalia Venditto • Apr 5 '21

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!

Lars Gyrup Brink Nielsen • Apr 5 '21 • 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.

Natalia Venditto • Apr 5 '21

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

That was my point.

Ray Suelzer • Mar 23 '21

"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();
    }
  };
}

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$))
   }
}

Lars Gyrup Brink Nielsen • Mar 23 '21

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.

Lars Gyrup Brink Nielsen • Mar 23 '21

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.

Ray Suelzer • Mar 24 '21

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.

Lars Gyrup Brink Nielsen • Mar 24 '21 • 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.

Giovanni Sarciotto • Mar 24 '21

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.

Lars Gyrup Brink Nielsen • Mar 24 '21

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.

Kyle Stephens • Mar 24 '21

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.

Mike Pearson • Mar 24 '21

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."

Lars Gyrup Brink Nielsen • Mar 24 '21

Thanks, Mike.

I just read your article dev.to/mfp22/isn-t-angular-reactiv...

Ethan Azariah • Apr 1 '21

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

Steve • Mar 30 '21

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

Kinanee Samson • Mar 23 '21

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

Lars Gyrup Brink Nielsen • Mar 23 '21 • Edited

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

SIMBIOSIS • Apr 3 '21

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.