For professional developers, code/peer review is an essential step of the internal QA. Over the years, among a wide variety of problems revealed on code reviews, I found that some appear more often than others. Here is a list of the most common ones.

So, the code is working, meets acceptance criteria, passed dev’s testing and here we go…

1. Lack of strong typing (using type ‘any’ implicitly or explicitly)

If the app is doomed to fail, it’s better to fail during the compilation. Therefore, all variables and methods’ input/output parameters need to be strongly typed.

Enforcing static type checks can be done via TSLint. Simply add ng lint into the building pipeline with specified no-any and typedef rules.

As an alternative (or in addition), tsconfig.json compiler options can also help. See noImplicitAny and noImplicitReturns parameters.

There is some resistance to enforcing strong types (often from devs with JavaScript background), as it requires writing a bit more code. However, this is worth higher reliability of the code and reduction in maintenance. As a rule of thumb, enforce types via interfaces, which are virtual structures (unlike classes) and only exist within the context of TypeScript without a footprint in the generated JavaScript.

Last thing to keep in mind, that strong type checks in TypeScript is not a panacea for all cases. Angular doesn’t check the types of component input parameters (@Input) and events (@Output). There are two open issues for that: #1 and #2. In some cases, devs may consider adding runtime type checks.

2. Function calls or impure pipes in HTML bindings

Usage of functions to bind values in the HTML markups is the most common reason for slow performed components. If you see abundance of bindings like <span> {{ foo() }} </span> (or used in a loop) in one component, you should raise a red flag. Note that getters behave like functions as well.

It’s important to remember that bindings with using functions or impure pipes are evaluated on every change detection cycle no matter whether the value or parameters change.

In general, encouraging using of OnPush Change Detection Strategy can help to reduce running unnecessary change detection cycles (see A Comprehensive Guide to Angular onPush Change Detection Strategy). And for catching impure pipes use a bundle of TSLint and Codelyzer (see no-pipe-impure rule).

3. Neglected RxJs subscriptions (no unsubscribe)

Perhaps, forgetting to unsubscribe RxJs subscriptions is the most popular mistake.

There are different ways how to manage RxJs subscriptions. The modern approaches are:

1. Force complete call on an Observable by either

   • Limiting the emitted items to the first one (by adding first() or take(1) into the pipeline);
   • Intruducing the takeUntil/takeWhile pattern (see ”Avoiding takeUntil Leaks” post from a RxJs team member).

   In this case, no need to unsubscribe unless having a cancelled subscription after ngOnDestroy is critical (as emitted items can come after the user navigated away from the component).

2. Compose subscriptions by using Subscription class. Add subscriptions to a collection

   this.subscription.add(observable1.subscribe(x => console.log('first: ' + x)))
   this.subscription.add(observable2.subscribe(x => console.log('second: ' + x)))

   and tear them down all at once in ngOnDestroy handler of the component by calling

   this.subscription.unsubscribe()

3. Use the async pipe in the HTML markup (| async).

   When the component gets destroyed, the async pipe unsubscribes automatically. They are especially handy when using OnPush Change Detection Strategy, as the async pipe marks the component to be checked for changes each time when a new value is emitted.

Can unsubscribing be enforced?

Unfortunately, there are no static analysis tools to enforce validation that all RxJs subscriptions are either complete or unsubscribed by the end of the life cycle of Angular component (when ngOnDestroy is called). rxjs-tslint-rules package has rxjs-no-unsafe-takeuntil rule, but it covers only the takeUntil pattern.

There are npm packages to automatically unsubscribe during ngOnDestroy (e.g. ngx-take-until-destroy and ngx-rx-collector), but it’s questionable how reliable they are.

Nested subscribes

A special case of mismanaging RxJs subscriptions, which requires attention, is nested subscribes:

observable1.subscribe(x => observable2.subscribe(...))

They make the code unreadable and introduces side effects (including potential memory leaks).

Usually, their presence indicates either lack of knowledge of RxJs mapping operators (switchMap, mergeMap, etc.) or poor architecture where RxJs subscribes are buried in nested function calls.

4. Misconfigured injectors of service providers

Unnecessary registration for app-wide scope

Often, devs generously register a service for the whole app even when it’s used in one place only. Think about the scope of your service providers and register them accordingly.

There are 3 options:

1. The root module (application-wide)

@Injectable({
  providedIn: 'root'
})

2. A specific module

@Injectable({
  providedIn: MyModule,
})

3. A specific component

@Component({
  providers: [MyService]
})

Duplicate instances of services in lazy-loaded modules

Mind a gotcha with lazy-loaded modules and services defined at the root and module levels:

Any component created within a lazy loaded module’s context, such as by router navigation, gets the local instance of the service, not the instance in the root application injector.

Confusion between The Old Way and The New Way of registering services

There is no need to register services in @NgModule anymore, just add @Injectable right on the service.

The @Injectable syntax (see examples above) was introduced in Angular 6 (a year ago, which is ages in the Angular world) and all devs need to switch to it, because it enables tree-shaking of the service if nothing injects it. I hope, the Angular team will remove the need for NgModules in future versions or at least will make them less annoying (pull request for that).

Still have questions? Read more in Hierarchical Dependency Injectors.

5. Lack of meaningful unit tests

Test stubs in the production code

Angular CLI encourages to write unit tests by spaning out *.spec.ts files with every created component. However, don’t leave them empty or be satisfied by configuring the TestBed with component initialisation without actual tests.

If devs don’t write tests, then absence of a test file would clearly indicate the state of affairs to other devs, rather than misleading them by giving a false sense of security with a rudimental *.spec.ts file.

Testing what’s easier to test, rather than fragile parts

Not all tests are equally valuable to the project. Any code (including tests) is maintenance burden and its presence needs to be justified.

Devs need to cover with tests the most fragile parts, rather than covering what’s easier to test. Pay attention to the inputs and outputs of the components, user interaction and used services. Gather some ideas from Angular Testing Recipes on GitHub.

6. Poorly structured CSS/SCSS

Excessive use of deep selectors (::ng-deep)

Usage of ::ng-deep to overwrite styles in other components is incredibly popular. In spite of being a working solution, it’s marked as deprecated since v4.3. Though, it isn’t going away until Angular implements ::part() and ::theme() from the CSS Shadow Parts spec (ticket for that), as there is no better alternative (see this discussion).

Regardless of its feature, ::ng-deep is used way more often than it should. At least consider alternatives before adding deep selectors:

1. :host-context when styles in your component depend on parents

   The :host-context() selector looks for a CSS class in any ancestor of the component host element, up to the document root. See example of applying styles based on some condition outside of a component’s view:

   :host-context(.theme-light) h2 {
   	background-color: #eef;
   }

2. Use globally-scoped styles sheets if need to overwrite the same style everywhere.

   Abstract overwritten styles into dedicated CSS/SCSS files included in styles.scss. Use high CSS specificity selectors to avoid unexpected leakage of the styles to other places. Don’t use ViewEncapsulation.None (see docs), as it’s the worst option (very prone to CSS leakage, which is hard to identify).

Having said that, weigh maintenance costs of alternatives carefully, ::ng-deep can be the least worst option. And if you are to use it, combine it with :host selector:

:host ::ng-deep h2 {
	background-color: #eef;
}

This will generate at runtime a style that looks like this

[_nghost-c0] h2 {
	background-color: #eef;
}

So this style will be applied to all h2 elements inside your component, but not outside of it.

Inline styles

While Angular is not complied with Content Security Policy (CSP) and generates inline <style> elements, which violates style-src (see open issue), having inline styles is still considered as bad practice due to poor scalability and maintainability (see other reasons).

As a rule of thumb, don’t be lazy and define all styles in the CSS/SCSS files.

Conclusion

We live in the world full of trade-offs and sometimes even the worst “mistake” can be justified. I hope, this article will encourage more considered and thoughtful code.

Please share your experience in the comments below, on Twitter or join the Reddit discussion.