🏷️ Interface and Typings Improvements.

[KingDarBoja]

As promised, I made a new branch containing only the type changes from my previous PR.

Feedback Changes - September 17.

Fixes #93.

📓 Take into account that with idField, rxfire now writes onto the object the converter returned, so a converter that returns a frozen object throws where the spread used to copy it.

Original Post

By the way, I managed to run the GH action for test and build locally using Nektos/act tool along with Docker so I could see the test suite results against the test matrix.

I really think the test suite workflow should be apart so it is easier to lookup at test results on pull requests so contributors do not have to wait for a maintainer to approve the jobs...

[armando-navarro]

Hey @KingDarBoja, thanks for submitting this. I found four things blocking approval, and the rest is optional.

Blocking: traceUntilFirst changed behavior

In performance/index.ts the rewrite ends the trace in the complete handler:

tap({
  complete: () => traceSubscription.unsubscribe(),
}),

• On main that unsubscribe is the next handler, so the trace ends at the first value, which is what the docblock describes.
• With this change it ends only when the source completes, which for a Firestore listener is never, and the operator becomes identical to traceUntilComplete right above it.
• Your #123 branch had next: here, so I think this slipped in while re-authoring. Changing complete: back to next: restores it. I ran all five operators through a jest spec that stubs window.performance, and the other four behave the same as on main.

Nothing in test/ covers the performance module, which is why the test jobs stayed green. A spec is not required for this PR, but if you do add one:

• import ... from '../performance' resolves through that folder's package.json to the built dist.
• Import from ../dist/performance instead, the way the other suites do.

Blocking: the Lint job

The 36 errors are all indentation or trailing whitespace, and yarn lint:fix clears every one of them with whitespace-only changes.

Blocking: collectionCountSnap now hands back a lite query

The full module's collectionCountSnap is typed with the lite SDK's AggregateQuerySnapshot:

• firestore/collection/index.ts imports CountSnapshot from ../lite/interfaces (that import predates you), so snap.query is a lite Query.
• On main that was hidden because the model slot was any.
• With AppModelType filled in, existing callers that use snap.query with the full SDK stop compiling.

Against a Query<Folk>:

• (await getDocs(snap.query)).docs[0].data() is now Folk | {name: string | FieldValue} instead of Folk.
• query(snap.query, limit(1)) is no longer a Query<Folk>.
• rxfire's own collectionData(snap.query) no longer emits Folk[].

All three compile on main. The fix that worked for me, on firebase 10 and 12: define the alias for the full module in firestore/interfaces.ts and import it from there instead of the lite file.

export type CountSnapshot<
  AppModelType = DocumentData,
  DbModelType extends DocumentData = DocumentData,
> = import('firebase/firestore').AggregateQuerySnapshot<
  {count: import('firebase/firestore').AggregateField<number>},
  AppModelType,
  DbModelType
>;

Blocking: the two-parameter lite Query alias under firebase 9

package.json still supports firebase 9, and firebase 9's lite SDK declares Query<T> with one parameter. Consumers compile against rxfire's emitted declarations, and with skipLibCheck on, under firebase 9 the new lite.Query<AppModelType, DbModelType> alias resolves to an error type there. The effect is silent:

• Against main's declarations, a firebase 9 project gets errors for collectionData(notAQuery), collection(notAQuery), and a wrong element type.
• Against this branch's declarations, the same file compiles with no errors, and every lite function typed with Query<T> accepts anything.

What worked for me on firebase 9, 10 and 12:

• Keep export type Query<T> = lite.Query<T>;.
• Give the two lite count functions the same one-parameter shape as the full module ones (Query<AppModelType> in, CountSnapshot<AppModelType> out).
• CountSnapshot can keep both parameters: its DbModelType defaults to DocumentData, and its firebase 9 problem is the pre-existing one from #94.

If you would rather drop firebase 9 from the peer range instead, that is a separate decision that would go in its own PR.

Optional: the new test under tsc

The docData converter test only compiles because jest runs through babel, which strips types without checking them.

• tsc over test/firestore.test.ts rejects it: Type '"UID"' is not assignable to type '"name"'.
• The reason is the signature docData<T, R extends T = T> with idField?: keyof R. With the converter, T is Folk, so the only legal key is name.
• Passing the generics fixes it and drops both casts from the test:

const unwrapped = docData<Folk, Folk & {UID: string}>(davidDoc.withConverter(Folk), {idField: 'UID'});

unwrapped.pipe(take(1)).subscribe((val) => {
  expect(val).toBeInstanceOf(Folk);
  expect(val).toEqual(expect.objectContaining({name: 'David', UID: 'david'}));
  done();
});

Optional: smaller things

• firestore/lite/collection/index.ts: the as Promise<CountSnapshot<AppModelType, DbModelType>> cast on getCount(query) is not needed. getCount already returns that type, and it compiles without the cast on firebase 10, 11 and 12.

• firestore/document/index.ts: the two casts in (data as Record<string, unknown>)[options.idField as string] = snapshot.id can go. Object.assign(data, {[options.idField]: snapshot.id}); type-checks and behaves the same.

• The lite module's snapToData follows the same write-in-place rule but has no converter-plus-idField test and no comment saying why. That is how the full copy went back to a spread in a 2022 return-type fix (ab8fe07) without anyone noticing until #93. A copy of your new test in test/firestore-lite.test.ts would keep it from happening again.

• traceUntil and traceWhile: since the PR rewrites those next handlers anyway, (a) => test(a) could become (value) => test(value).

• docs/storage.md line 100 still lists getMetadata() as returning Observable<Object>, and the docs folder ships in the package. Observable<import('firebase/storage').FullMetadata> matches the rows below it. The description above it also says "emits the URL of the file's metadta", which was wrong before you got here.

• Please add Fixes #93 to the description, since this resolves it. One behavior note worth a sentence there too: with idField, rxfire now writes onto the object the converter returned, so a converter that returns a frozen object throws where the spread used to copy it (the lite module already works that way).

On the workflow question in your description:

• GitHub decides whether a fork PR's runs need approval per pull request, from the contributor's status under the repo's Actions settings, not per workflow file, so moving the test job to its own file would not change it.
• Unless the repo is set to require approval for all external contributors, the requirement ends once you have a merged PR, which this one will be.

Thanks again. If I have misread the traceUntilFirst change, tell me and I will look again.

[KingDarBoja]

Alright, I believe I have made all the changes (even the optional ones) from your feedback. @armando-navarro