Repository navigation
Add support for Firestore pipeline operations - #3799
KingDarBoja wants to merge 2 commits into
Conversation
|
Thank you for submitting this. This was on my plan of things to add. There's actually so much more than this that needs to be included from Firebase. We'll get this done together. Thank you again. |
armando-navarro
left a comment
There was a problem hiding this comment.
Hi @KingDarBoja, thanks again, and sorry for taking this long to review this. I had prioritized the AngularFire 22 planning, but now that that's done, and the AngularFire 22 PRs are going to roll out swiftly as a result, I felt it was time for me to get to your PR.
There are three things I'd like changed before approving, plus one optional item.
Lite: pipeline() doesn't type-check
With Firestore from @angular/fire/firestore/lite, inject(Firestore).pipeline() fails to compile:
- The error is
TS2339: Property 'pipeline' does not exist on type 'Firestore'. - The cause is in firebase's typings. The types its exports map serves for the Lite pipelines path leave out the
pipeline()declaration, so importing straight fromfirebase/firestore/lite/pipelinesfails the same way. - At runtime the method is there. Only the type is missing.
Since the docs point Lite users at this entry point, I think we should add the declaration ourselves. With this in src/firestore/lite/pipelines/public_api.ts, the package builds and the Lite call compiles:
import type { Pipeline, PipelineSource } from 'firebase/firestore/lite/pipelines';
declare module '@firebase/firestore/lite' {
interface Firestore {
pipeline(): PipelineSource<Pipeline>;
}
}
export * from './firebase';Docs
The new section shows only the imports. Could you add:
- A short example that gets a pipeline from the injected
Firestorewithfirestore.pipeline()and passes it toexecute(). - A note that pipelines need an Enterprise edition database. Firebase's edition comparison says Standard edition is "Limited to Firestore Core operations".
PR description
- "Issue number for this PR: #3693" won't close the issue when this merges.
Fixes #3693will. - Please add
Refs #3755as well, since this covers 2 of the 6 missing entry points listed there.
Optional: a spec for the Lite fix
Could you add a spec that calls pipeline() on the injected Firestore from both entry points, so the Lite fix can't regress? The Lite one fails to compile without the declaration above and passes with it.
Once these are in, mark the PR ready for review and I'll take another look. If I've misread anything here, let me know.
|
@armando-navarro take into account before merging that this PR is pointing to |
Checklist
firebaseentry points have no@angular/fireequivalent, so their exports are unreachable #3755yarn install,yarn testrun successfully? YesDescription
Implement Firestore Pipelines (including lite version).
Code sample