feat(functions): Adding AngularFireFunctions with httpCallable #1532
Conversation
There was a problem hiding this comment.
Looks good @jamesdaniels! Just a few comments about new RxJS import paths and using pipeable operators.
src/functions/functions.ts
Outdated
|
|
||
| import { FirebaseAppConfig, FirebaseAppName, _firebaseAppFactory, FirebaseZoneScheduler } from 'angularfire2'; | ||
|
|
||
| import 'rxjs/add/observable/fromPromise'; |
There was a problem hiding this comment.
Make sure to use pipeable operators.
src/functions/functions.ts
Outdated
| const callable = this.functions.httpsCallable(name); | ||
| return (data: T) => { | ||
| return this.scheduler.runOutsideAngular( | ||
| Observable.fromPromise(callable(data)) |
There was a problem hiding this comment.
Use import { from } from 'rxjs/observable/from';. It also works with promises. Then use the rxjs/operators specifier to import the pipeable operators.
import { from } from 'rxjs/observable/from';
import { map } from 'rxjs/operators';
from(promise).pipe(
map(() => {})
);
src/functions/functions.ts
Outdated
| } | ||
| } | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Nit. Bring the constructor above the method above.
src/functions/functions.ts
Outdated
| import { FirebaseFunctions, HttpsCallableResult } from '@firebase/functions-types'; | ||
| import { FirebaseOptions } from '@firebase/app-types'; | ||
| import { Injectable, Inject, Optional, NgZone } from '@angular/core'; | ||
| import { Observable } from 'rxjs/Observable'; |
There was a problem hiding this comment.
You can now just do
import { Observable } from 'rxjs';
src/functions/functions.ts
Outdated
| return (data: T) => { | ||
| return this.scheduler.runOutsideAngular( | ||
| Observable.fromPromise(callable(data)) | ||
| .map(r => r.data as R) |
There was a problem hiding this comment.
@davideast what do you think about mapping .data here? This is the only thing I'm on the fence about. It's nice ergo buuut what if the CF3 team decides to pass more than .data back at some point? That would force an API break.
|
Appreciate the review, I'll jump on these after addressing the Firebase SDK stuff. |
jamesdaniels
changed the title
jamesdaniels
changed the title
|
@davideast addressed your review and updated against master. |
Checklist
yarn install,yarn testrun successfully? (yes/no; required)Description
Code sample