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

Dependency injection writeup #333

Merged
merged 25 commits into from
May 8, 2024
Merged
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 219 additions & 0 deletions docs/architecture/clients/dependency-injection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
---
sidebar_position: 4
---

# Dependency Injection

Dependency injection (DI) in our Typescript clients is manually configured across several locations.

You will need to update the DI configuration when you create a new service or change the
dependencies injected into an existing service.

## Angular and Non-Angular contexts

Our Typescript clients use both Angular and non-Angular contexts. This can vary:

- between apps - for example, the web client is entirely Angular, whereas the CLI client does not
use Angular at all
- within apps - for example, the browser client uses Angular in its popup but not in its background
page/service worker

This has important consequences for how we write services and how we configure DI. In particular:

- Angular contexts use the Angular dependency injection framework
- non-Angular contexts do not use any dependency injection framework and must manually instantiate
their dependencies
MGibson1 marked this conversation as resolved.
Show resolved Hide resolved

## Services

Services should be organized as follows depending on where they're used:

- `libs/common` for services used by **multiple clients** in **non-Angular contexts (or a mix of
Angular and non-Angular contexts)**
- `libs/angular` for services used by **multiple clients** in **Angular contexts only**
- `apps/<client-name>` for services used by a single client only
Comment on lines +36 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 suggestion: mention the team-specific libraries e.g. libs/auth, libs/vault


If a service is used in an Angular context only, it can use the Angular `@Injectable` decorator to
automatically configure its dependencies. If a service is used in mixed contexts (e.g.
`libs/common`), it must not use Angular decorators and its dependencies must be manually configured.
MGibson1 marked this conversation as resolved.
Show resolved Hide resolved

## Interfaces

A service should implement an interface defined by an abstract class if:

1. The service will be used across multiple client apps, or
2. You want to define multiple interfaces for your service to limit what functionality is available
to different consumers
MGibson1 marked this conversation as resolved.
Show resolved Hide resolved

For example, you may want to use interfaces to prevent outside modules from calling your internal
methods:

```ts
// Injected in other modules
abstract class MyService {
externalMethod: () => void;
}

// Injected in the MyService feature module
abstract class InternalMyService extends MyService {
internalMethod: () => void;
}
coroiu marked this conversation as resolved.
Show resolved Hide resolved

class MyService implements InternalMyService {
externalMethod: () => void;
internalMethod: () => void;
}
```

If neither of these apply to you, you probably don't need to define an abstract interface for your
service.
MGibson1 marked this conversation as resolved.
Show resolved Hide resolved

## Configuring DI

### Angular modules

Angular contexts generally use ngModules
[dependency providers](https://angular.io/guide/dependency-injection-providers) to configure DI. For
example:

coroiu marked this conversation as resolved.
Show resolved Hide resolved
```ts
@NgModule({
providers: [
// If the service does not have @Injectable decorators
{
provide: MyService, // abstract interface
useClass: DefaultMyService, // concrete implementation
deps: [DepA, DepB], // abstract interfaces for each dependency required by the DefaultMyService constructor
},
// If the service does have @Injectable decorators (Angular-only)
{
provide: SomeAngularService,
useClass: DefaultSomeAngularService,
// no deps array required
},
],
})
export class MyServicesModule {}
```

By default, this is not type safe - Angular does not verify that the implementation actually
implements the abstract interface, or that the `deps` array matches the constructor parameters.

We provide a helper function called `safeProvider`, which acts as a wrapper around each provider
entry and provides compile-time type checking of your configuration. For example (continuing with
the example above):

```ts
@NgModule({
providers: [
safeProvider({
provide: MyService,
useClass: DefaultMyService,
deps: [DepA, DepB],
}),
safeProvider({
provide: SomeAngularService,
useClass: DefaultSomeAngularService,
useAngularDecorators: true, // this tells safeProvider to not require a deps array
}),
],
})
export class MyServicesModule {}
```

:::note

The `useAngularDecorators` option is not type safe - it is your responsibility to ensure that the
class actually uses the `@Injectable` decorator.

:::
Comment on lines +145 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 thought: we should keep an eye on microsoft/TypeScript#4881 which might be able to fix this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice spot!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this open for future reference! :)


Our primary service modules require the use of `safeProvider`, otherwise you will receive type
errors. However, you should use `safeProvider` wherever you are configuring an Angular provider to
benefit from its type checking.

### Angular root module

Services that will be used globally and which don't have an abstract interface can be registered
using the `provideIn` property. This avoids having to create and manage service modules. When
exporting services through feature modules, it can be tricky to manage their lifetime, and it's easy
to end up with instances of the service being created.
eliykat marked this conversation as resolved.
Show resolved Hide resolved

```ts
@Injectable({
provideIn: "root",
})
export class MyService {}
```

### Angular components

Services that do are not stateful and/or are only supposed to be used locally to extract complex
eliykat marked this conversation as resolved.
Show resolved Hide resolved
logic from a component can be injected directly into standalone components, completely bypassing
modules.

```ts
@Component({
providers: [
// This example assumes that MyService uses @Injectable decorators and does not have an abstract interface
safeProvider(MyService),
coroiu marked this conversation as resolved.
Show resolved Hide resolved
],
standalone: true,
})
export class MyComponent {}
```

### Non-Angular contexts

Non-Angular contexts do not use a DI framework and must manually instantiate their dependencies **in
eliykat marked this conversation as resolved.
Show resolved Hide resolved
the correct order**. Instantiating dependencies out of order may result in null values being
injected into services, causing runtime errors.
coroiu marked this conversation as resolved.
Show resolved Hide resolved

## DI by client

### Shared Angular DI

`libs/angular` contains `jslib-services.module.ts`, a services module which registers common
eliykat marked this conversation as resolved.
Show resolved Hide resolved
dependencies used across Angular Typescript clients (web, browser and desktop).

A client may override this DI if a client-specific configuration is required.

### Web

`core.module.ts` contains the main services module for the web client. It imports
`JslibServicesModule`.

However, the web module heavily uses [feature modules](https://angular.io/guide/feature-modules) to
divide its code by feature. Feature modules should configure their own DI wherever possible for
feature-specific (non-global) services.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): preferably in standalone components or using provideIn: 'root' and not in the actual feature module

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? If the component is standalone, then by definition it doesn't have a module, so advice about a feature module doesn't apply. And if you have a service that is only used in a feature module, I assume you'd want to register it in the more specific location rather than in root. Does this go to your earlier concern about accidentally instantiating duplicate services?

Copy link
Contributor

@coroiu coroiu Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the component is standalone, then by definition it doesn't have a module, so advice about a feature module doesn't apply.

Good point, I read feature module as just a folder in an organize-by-feature structure. I think feature modules could (and should) in most cases be replaced by standalone component, but that discussion might be outside the scope of this PR comment :D

Does this go to your earlier concern about accidentally instantiating duplicate services?

Yes, and other things. It depends a little bit on how we define a "feature". If feature => 1 exported top-level component then we are basically talking about standalone components.

In the case where 1 feature module exports multiple components then the module becomes a bit rigid. It's a big blob containing all of the dependencies that any of it's child components might need. It makes it less obvious which component needs which service/pipe/etc. and also affects tree-shaking negatively. The lifetime of the service also becomes much easier to reason about, since it's tied to an actual DOM component, instead of a module that is somewhat magically instantiated by Angular in the background. Service tied to components also have access to the activated route in a way that service in modules never will (though this use-case might be a bit of an edge case).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a mention of standalone components when discussing the web vault client, otherwise I suggest this could be done in a subsequent revision of this page - which would definitely be welcome. It might require some discussion/consensus on stricter rules around feature modules though. I agree in many cases they can be replaced with standalone, but it's not what we do today.


### Browser

Browser DI is split across the following locations:

- `services.module.ts` is the main services module for the browser extension popup. It imports
`JslibServicesModule`
- `main.background.ts` manually instantiates services used in the background page (for manifest v2)
or the service worker (for manifest v3). It does not use any dependency injection framework
- a series of exported factory functions with the naming convention `[name]-service.factory.ts`.
These may be used in manifest v3 service workers in the future
MGibson1 marked this conversation as resolved.
Show resolved Hide resolved

The background page/service worker still does a lot of work in the browser extension, so many
services are registered in all the above locations.

### Desktop

Desktop DI is split across the following locations:

- `services.module.ts` is the main services module for the Electron renderer process. It imports
`JslibServicesModule`
- `main.ts` manually instantiates services used in the main process. It does not use any dependency
injection framework

The main process is primarily used for Electron behavior and native integrations (e.g. storage,
biometrics, window behavior), so it registers a smaller subset of services. Many services are only
registered in the renderer process.
coroiu marked this conversation as resolved.
Show resolved Hide resolved

### CLI

`bw.ts` contains all DI for the CLI client. It does not use any dependency injection framework.