[AC-2278] [BEEEP] Typesafe Angular DI #8206
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8206 +/- ##
=======================================
Coverage 25.70% 25.70%
=======================================
Files 2267 2268 +1
Lines 66307 66310 +3
Branches 12454 12452 -2
=======================================
+ Hits 17045 17046 +1
- Misses 47912 47914 +2
Partials 1350 1350 ☔ View full report in Codecov by Sentry. |
|
New Issues
|
There was a problem hiding this comment.
This class used both injectable decorators (to inject its dependencies) and the providers array (to match the abstraction to the implementation). This is the only class that mixes these approaches and I think it's confusing. In any case, it couldn't be reconciled with this PR, so I've changed it to only use the providers array.
| * of the same name. This ensures that your definition is typesafe. | ||
| * If you need help please ask for it, do NOT change the type of this array. | ||
| */ | ||
| const typesafeProviders: Array<SafeProvider | Constructor<any>> = [ |
There was a problem hiding this comment.
SafeProvider is defined as Opaque<Provider> in the helpers file. It's the type returned by all our helper functions, and it's an attempt to make people use them rather than continue to add object literals.
It's a bit awkward and I'm not entirely happy with it but I think it's the simplest solution compared to writing an eslint rule or something. However, any suggestions for how this can be improved are welcome - particularly so that other team members can understand what we're doing here.
There was a problem hiding this comment.
This file is reviewable if you use the "Hide whitespace" option in Github. And you should review this - I did have to change (fix) some dependencies. (My favourite is where we were injecting LoginService instead of LogService)
There was a problem hiding this comment.
I've committed @MGibson1' contribution, which combines the factory functions into a single factory function (thanks Matt!), and I've made some further changes. The commit history is relatively clean if you prefer to review that way.
I moved this new work under Platform ownership - it fits their responsibilities and I also want a clear owner that people can bug with questions or change requests. (You're welcome ;) )
I can extend this pattern to our other services modules in a separate PR after this is merged.
| /** | ||
| * Represents a dependency provided with the useExisting option. | ||
| */ | ||
| type SafeExistingProvider< | ||
| A extends Constructor<any> | AbstractConstructor<any>, | ||
| I extends Constructor<InstanceType<A>> | AbstractConstructor<InstanceType<A>>, | ||
| > = { | ||
| provide: A; | ||
| useExisting: I; | ||
| }; |
There was a problem hiding this comment.
I've tightened up the type check here by removing Partial.
This is because I decided that our current practice was wrong. We should register the Internal version of each service first, and then use useExisting to register the more restricted (non-internal) interface. That way we can guarantee that the implementation type extends the abstraction type in the useExisting declaration. See 10d4250
There was a problem hiding this comment.
I agree that we were doing it backwards, but I think even better would be to, for example
safeProvider({
provide: OrganizationService,
useClass: OrganizationService,
deps: [StateServiceAbstraction, StateProvider],
}),
safeProvider({
provide: InternalOrganizationServiceAbstraction,
useExisting: OrganizationService,
}),
safeProvider({
provide: OrganizationServiceAbstraction,
useExisting: OrganizationService,
}),The hope here is that we don't actually need the InternalOrganizationService to extend OrganizationService so we could implement arbitrary interfaces and we can use the single class to provide a slew of non-interlocking interfaces.
| /** | ||
| * Represents a dependency provided with the useFactory option where a SafeInjectionToken is used as the token. | ||
| */ | ||
| type SafeFactoryProviderWithToken< | ||
| A extends SafeInjectionToken<any>, | ||
| I extends (...args: any) => InstanceType<SafeInjectionTokenType<A>>, | ||
| D extends MapParametersToDeps<Parameters<I>>, | ||
| > = { | ||
| provide: A; | ||
| useFactory: I; | ||
| deps: D; | ||
| }; | ||
|
|
||
| /** | ||
| * Represents a dependency provided with the useFactory option where an abstract class is used as the token. | ||
| */ | ||
| type SafeFactoryProviderWithClass< | ||
| A extends AbstractConstructor<any>, | ||
| I extends (...args: any) => InstanceType<A>, | ||
| D extends MapParametersToDeps<Parameters<I>>, | ||
| > = { | ||
| provide: A; | ||
| useFactory: I; | ||
| deps: D; | ||
| }; |
There was a problem hiding this comment.
I have split SafeFactoryProvider into 2 separate types, 1 where an InjectionToken is used, and 1 where an Abstract class is used. This means the types are kept dead simple with no conditional extends logic. The drawback is that we add yet more types to the safeProvider function. I thought this was an acceptable tradeoff but I'm open to other views here.
There was a problem hiding this comment.
I'm fine with either. The only drawback is more generics on safeProvider and realistically that ship has sailed. If you ever need to specify types on that thing this isn't going to work.
| * @param provider Your provider object in the usual shape (e.g. using useClass, useValue, useFactory, etc.) | ||
| * @returns The exact same object without modification (pass-through). | ||
| */ | ||
| export const safeProvider = < |
There was a problem hiding this comment.
Some of the types here are duplicates, but I think it's preferable to keep them clearly separated (if duplicative) so that they can easily be updated in step with any changes to the underlying types.
eliykat
changed the title
| * Represents a dependency provided with the useFactory option where an abstract class is used as the token. | ||
| */ | ||
| type SafeFactoryProviderWithClass< | ||
| A extends AbstractConstructor<any>, |
There was a problem hiding this comment.
Isn't it valid to use a non-abstract constructor here, too?
| * Represents a dependency provided with the useClass option. | ||
| */ | ||
| type SafeClassProvider< | ||
| A extends AbstractConstructor<any>, |
There was a problem hiding this comment.
Isn't it valid to use a non-abstract constructor here, too?
Type of change
Objective
A proposal for making our Angular DI configuration typesafe. I recommend checking this out locally and breaking stuff to convince yourself that it provides the safety I think it does.
The basic outline is:
{provide: myAbstractClass, useClass: myClass, deps: []}), which can quickly become out of date or incorrect, we now use factory functions that correspond to each method of registering a dependency: useClass, useValue, etc.depsarray matches what the implementation needs in its constructor.SafeProvider.jslib-services.modulenow requires that the providers array be of this type, so you can no longer use object literals.Code changes
Screenshots
Before you submit