Dependency Inject Homepage Firestore #735
Conversation
|
|
||
| abstract getRestaurantCollectionData(): Observable<Restaurant[]>; | ||
|
|
||
| abstract getRestaurntsGivenConstraints(constraints: QueryConstraint[]): Observable<Restaurant[]>; |
There was a problem hiding this comment.
nit: line length exceeds 80 - and a few more lines below
| } | ||
|
|
||
| @Injectable() | ||
| export class MockHomepageFirestore extends HomepageFirestore { |
There was a problem hiding this comment.
We shouldn't colocate "real" classes with fake ones. This would be better suited to live inside the test class (spec.ts).
One additional benefit there is that the test data is homed closer (as it should always be IMO) to the tests being run to make understanding the test easier.
There was a problem hiding this comment.
Gotcha; Moved to homepage.component.spec.ts!
|
|
||
|
|
||
| @Injectable() | ||
| export class DefaultHomepageFirestore extends HomepageFirestore { |
There was a problem hiding this comment.
nit: comment liberally why this is a service, why it exists, what its function is (wrt testing and injection)
| */ | ||
|
|
||
| @Injectable() | ||
| export abstract class HomepageFirestore { |
There was a problem hiding this comment.
Is an interface possible?
There was a problem hiding this comment.
It seems like an interface wouldn't be @Injectable, but I've managed to move the constructor out of the abstract class in an upcoming commit!
| @Injectable() | ||
| export abstract class HomepageFirestore { | ||
| store: Firestore; | ||
| constructor(store: Firestore) { |
There was a problem hiding this comment.
Even if we need to use an abstract class, let's move the constructor out
| return of(mockRestaurants); | ||
| } | ||
|
|
||
| override getRestaurntsGivenConstraints( |
There was a problem hiding this comment.
Typo restaurnts
This PR Adds a dependency injected service to the
src/app/homepage/homepage.component.tsfile that abstracts away Firestore calls to make the component more compatible with Unit Testing.