Dependency injection writeup

[eliykat]

Objective

My initial goal here was to document safeProvider, however I decided that we needed a writeup about our DI patterns in general to properly contextualize it. This was tricky to structure, partly because our patterns in this area are varied and diffuse, so feedback is particularly welcome.

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 9e10c7d6-47a0-4b0a-930d-d91d0eb6c903

No New Or Fixed Issues Found

[coroiu]

I think there are some additional subtleties that we should probably mention here. This method of configuring DI services is mainly used for multi-client services. For single-client services and/or angular-only services, we should also mention:

provideIn

Services that are intended to be used globally can use the provideIn property which 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.

@Injectable({
    provideIn: "root"
})

Standalone components

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

@Component({
    providers: [SomeService],
    standalone: true
})

[eliykat]

providedIn assumes that there is no abstract interface for the service, right? Do we have any clear guidelines/rules as to when that's appropriate? The current practice seems to be "small, Angular-only services don't need abstractions if you don't feel like it" but I'm not really sure why.

Perhaps abstract interfaces are only used to manage different implementations across apps, and if a service is for a single app only, it doesn't need one? (This seems to be the approach SM Team have taken, I see providedIn: "root" for all their services.)

[coroiu]

providedIn assumes that there is no abstract interface for the service, right?

Exactly!

Perhaps abstract interfaces are only used to manage different implementations across apps, and if a service is for a single app only, it doesn't need one?

This is the same approach I've been using! The component library might also export services without abstractions, not sure if they use providedIn though, but they probably should!

[eliykat]

After discussing this on Slack, I've incorporated your suggestions. Unfortunately there are now a lot of options for how to register a service in Angular - but that reflects our current practice. Open to any further feedback re: structure or being more opinionated in the text.

[cloudflare-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	7ea529f
Status:	✅  Deploy successful!
Preview URL:	https://a6c15a16.contributing-docs.pages.dev
Branch Preview URL:	https://dependency-injection.contributing-docs.pages.dev

View logs

[coroiu]

I gave all of the parts a detailed read-through this time, and while I did have quite a few comments, it's all mostly just polish and I would've ok with merging as-is.

[coroiu]

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

[coroiu]

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

[eliykat]

Nice spot!

[coroiu]

Keeping this open for future reference! :)

[coroiu]

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

[eliykat]

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?

[coroiu]

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

[eliykat]

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.

[eliykat]

Thanks for all the feedback @coroiu , I've addressed or responded to everything.

Can I please have a review from @bitwarden/dept-architecture as codeowners as well, thanks.

[coroiu]

Looking good! One final issue

[coroiu]

Keeping this open for future reference! :)

[MGibson1]

Responded to comments. Only thing really still outstanding is the factory method statements.