Skip to content

Firestore: QoL improvements for converters #5268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Aug 18, 2021
Merged

Firestore: QoL improvements for converters #5268

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f5a00fc
WIP: working NestedParial and WithFieldValue
Aug 10, 2021
e87b617
add support for TypedUpdateData + sanity tests
Aug 10, 2021
fdd432a
add support for WriteBatch and Transaction
Aug 10, 2021
01e1529
incremental fixes
Aug 11, 2021
1d48049
fix exports/imports for exp and lite
Aug 11, 2021
de10404
Merge branch 'master' into bc/types
Aug 11, 2021
d2df5e8
Fix docs and imports
Aug 11, 2021
21cc4bf
Fixing imports
Aug 11, 2021
c5bb26f
move ts-lint
Aug 11, 2021
c2e0e38
add comments and skip test
Aug 12, 2021
479b6bb
trying fei's rollup hack
Aug 12, 2021
1ffaad2
allow skip
Aug 12, 2021
eb7d26c
merge and rename
Aug 16, 2021
b0568bb
revert rollup changes
Aug 16, 2021
826fff2
add integration tests
Aug 17, 2021
d94b0f4
Merge branch 'master' into bc/types
Aug 17, 2021
0b620ca
revert auth-exp lint
Aug 17, 2021
da10a15
resolve comments
Aug 17, 2021
cf41539
move helpers types to separate file
Aug 17, 2021
a2df21b
add changeset
Aug 17, 2021
c11e067
update comments and changeset
Aug 17, 2021
706c060
some comment changes
Aug 17, 2021
a8123d8
Update API reports
thebrianchen Aug 17, 2021
f9d4b3e
fix addDoc test
Aug 18, 2021
416850e
Merge branch 'bc/types' of github.com:firebase/firebase-js-sdk into b…
Aug 18, 2021
5a175a4
update comments
Aug 18, 2021
635c9e3
Merge branch 'master' into bc/types
Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions common/api-review/firestore-lite.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ import { FirebaseApp } from '@firebase/app-exp';
import { LogLevelString as LogLevel } from '@firebase/logger';

// @public
export function addDoc<T>(reference: CollectionReference<T>, data: T): Promise<DocumentReference<T>>;
export function addDoc<T>(reference: CollectionReference<T>, data: WithFieldValue<T>): Promise<DocumentReference<T>>;

// @public
export type AddPrefixToKeys<Prefix extends string, T extends Record<string, any>> = {
[K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
};

// @public
export function arrayRemove(...elements: unknown[]): FieldValue;
Expand Down Expand Up @@ -132,8 +137,8 @@ export class Firestore {
// @public
export interface FirestoreDataConverter<T> {
fromFirestore(snapshot: QueryDocumentSnapshot<DocumentData>): T;
toFirestore(modelObject: T): DocumentData;
toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
toFirestore(modelObject: WithFieldValue<T>): DocumentData;
toFirestore(modelObject: NestedPartial<T>, options: SetOptions): DocumentData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to align those two names. Have you thought of a naming pattern that would achieve that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to PartialWithFieldValue.

}

// @public
Expand Down Expand Up @@ -182,12 +187,25 @@ export function limitToLast(limit: number): QueryConstraint;

export { LogLevel }

// @public
export type NestedPartial<T> = T extends Primitive ? T : T extends Map<infer K, infer V> ? Map<NestedPartial<K>, NestedPartial<V>> : T extends {} ? {
[K in keyof T]?: NestedPartial<T[K]> | FieldValue;
} : Partial<T>;

// @public
export type NestedUpdateFields<T extends Record<string, any>> = UnionToIntersection<{
[K in keyof T & string]: T[K] extends Record<string, any> ? AddPrefixToKeys<K, UpdateData<T[K]>> : never;
}[keyof T & string]>;

// @public
export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryConstraint;

// @public
export type OrderByDirection = 'desc' | 'asc';

// @public
export type Primitive = string | number | boolean | undefined | null;

// @public
export class Query<T = DocumentData> {
protected constructor();
Expand Down Expand Up @@ -237,10 +255,10 @@ export function runTransaction<T>(firestore: Firestore, updateFunction: (transac
export function serverTimestamp(): FieldValue;

// @public
export function setDoc<T>(reference: DocumentReference<T>, data: T): Promise<void>;
export function setDoc<T>(reference: DocumentReference<T>, data: WithFieldValue<T>): Promise<void>;

// @public
export function setDoc<T>(reference: DocumentReference<T>, data: Partial<T>, options: SetOptions): Promise<void>;
export function setDoc<T>(reference: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): Promise<void>;

// @public
export function setLogLevel(logLevel: LogLevel): void;
Expand Down Expand Up @@ -302,19 +320,22 @@ export class Timestamp {
export class Transaction {
delete(documentRef: DocumentReference<unknown>): this;
get<T>(documentRef: DocumentReference<T>): Promise<DocumentSnapshot<T>>;
set<T>(documentRef: DocumentReference<T>, data: T): this;
set<T>(documentRef: DocumentReference<T>, data: Partial<T>, options: SetOptions): this;
update(documentRef: DocumentReference<unknown>, data: UpdateData): this;
set<T>(documentRef: DocumentReference<T>, data: WithFieldValue<T>): this;
set<T>(documentRef: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): this;
update<T>(documentRef: DocumentReference<T>, data: UpdateData<T>): this;
update(documentRef: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): this;
}

// @public
export interface UpdateData {
[fieldPath: string]: any;
}
export type UnionToIntersection<U> = (U extends any ? (k: U) => void : never) extends (k: infer I) => void ? I : never;

// @public
export function updateDoc(reference: DocumentReference<unknown>, data: UpdateData): Promise<void>;
export type UpdateData<T> = T extends Primitive ? T : T extends Map<infer K, infer V> ? Map<UpdateData<K>, UpdateData<V>> : T extends {} ? {
[K in keyof T]?: UpdateData<T[K]> | FieldValue;
} & NestedUpdateFields<T> : Partial<T>;

// @public
export function updateDoc<T>(reference: DocumentReference<T>, data: UpdateData<T>): Promise<void>;

// @public
export function updateDoc(reference: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): Promise<void>;
Expand All @@ -325,13 +346,18 @@ export function where(fieldPath: string | FieldPath, opStr: WhereFilterOp, value
// @public
export type WhereFilterOp = '<' | '<=' | '==' | '!=' | '>=' | '>' | 'array-contains' | 'in' | 'array-contains-any' | 'not-in';

// @public
export type WithFieldValue<T> = T extends Primitive ? T : T extends {} ? {
[K in keyof T]: WithFieldValue<T[K]> | FieldValue;
} : Partial<T>;

// @public
export class WriteBatch {
commit(): Promise<void>;
delete(documentRef: DocumentReference<unknown>): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: T): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: Partial<T>, options: SetOptions): WriteBatch;
update(documentRef: DocumentReference<unknown>, data: UpdateData): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: WithFieldValue<T>): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): WriteBatch;
update<T>(documentRef: DocumentReference<T>, data: UpdateData<T>): WriteBatch;
update(documentRef: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): WriteBatch;
}

Expand Down
56 changes: 41 additions & 15 deletions common/api-review/firestore.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ import { FirebaseApp } from '@firebase/app-exp';
import { LogLevelString as LogLevel } from '@firebase/logger';

// @public
export function addDoc<T>(reference: CollectionReference<T>, data: T): Promise<DocumentReference<T>>;
export function addDoc<T>(reference: CollectionReference<T>, data: WithFieldValue<T>): Promise<DocumentReference<T>>;

// @public
export type AddPrefixToKeys<Prefix extends string, T extends Record<string, any>> = {
[K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
};

// @public
export function arrayRemove(...elements: unknown[]): FieldValue;
Expand Down Expand Up @@ -162,8 +167,8 @@ export class Firestore {
// @public
export interface FirestoreDataConverter<T> {
fromFirestore(snapshot: QueryDocumentSnapshot<DocumentData>, options?: SnapshotOptions): T;
toFirestore(modelObject: T): DocumentData;
toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
toFirestore(modelObject: WithFieldValue<T>): DocumentData;
toFirestore(modelObject: NestedPartial<T>, options: SetOptions): DocumentData;
}

// @public
Expand Down Expand Up @@ -256,6 +261,16 @@ export { LogLevel }
// @public
export function namedQuery(firestore: Firestore, name: string): Promise<Query | null>;

// @public
export type NestedPartial<T> = T extends Primitive ? T : T extends Map<infer K, infer V> ? Map<NestedPartial<K>, NestedPartial<V>> : T extends {} ? {
[K in keyof T]?: NestedPartial<T[K]> | FieldValue;
} : Partial<T>;

// @public
export type NestedUpdateFields<T extends Record<string, any>> = UnionToIntersection<{
[K in keyof T & string]: T[K] extends Record<string, any> ? AddPrefixToKeys<K, UpdateData<T[K]>> : never;
}[keyof T & string]>;

// @public
export function onSnapshot<T>(reference: DocumentReference<T>, observer: {
next?: (snapshot: DocumentSnapshot<T>) => void;
Expand Down Expand Up @@ -317,6 +332,9 @@ export interface PersistenceSettings {
forceOwnership?: boolean;
}

// @public
export type Primitive = string | number | boolean | undefined | null;

// @public
export class Query<T = DocumentData> {
protected constructor();
Expand Down Expand Up @@ -368,10 +386,10 @@ export function runTransaction<T>(firestore: Firestore, updateFunction: (transac
export function serverTimestamp(): FieldValue;

// @public
export function setDoc<T>(reference: DocumentReference<T>, data: T): Promise<void>;
export function setDoc<T>(reference: DocumentReference<T>, data: WithFieldValue<T>): Promise<void>;

// @public
export function setDoc<T>(reference: DocumentReference<T>, data: Partial<T>, options: SetOptions): Promise<void>;
export function setDoc<T>(reference: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): Promise<void>;

// @public
export function setLogLevel(logLevel: LogLevel): void;
Expand Down Expand Up @@ -446,24 +464,27 @@ export class Timestamp {
export class Transaction {
delete(documentRef: DocumentReference<unknown>): this;
get<T>(documentRef: DocumentReference<T>): Promise<DocumentSnapshot<T>>;
set<T>(documentRef: DocumentReference<T>, data: T): this;
set<T>(documentRef: DocumentReference<T>, data: Partial<T>, options: SetOptions): this;
update(documentRef: DocumentReference<unknown>, data: UpdateData): this;
set<T>(documentRef: DocumentReference<T>, data: WithFieldValue<T>): this;
set<T>(documentRef: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): this;
update<T>(documentRef: DocumentReference<T>, data: UpdateData<T>): this;
update(documentRef: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): this;
}

// @public
export type UnionToIntersection<U> = (U extends any ? (k: U) => void : never) extends (k: infer I) => void ? I : never;

// @public
export interface Unsubscribe {
(): void;
}

// @public
export interface UpdateData {
[fieldPath: string]: any;
}
export type UpdateData<T> = T extends Primitive ? T : T extends Map<infer K, infer V> ? Map<UpdateData<K>, UpdateData<V>> : T extends {} ? {
[K in keyof T]?: UpdateData<T[K]> | FieldValue;
} & NestedUpdateFields<T> : Partial<T>;

// @public
export function updateDoc(reference: DocumentReference<unknown>, data: UpdateData): Promise<void>;
export function updateDoc<T>(reference: DocumentReference<T>, data: UpdateData<T>): Promise<void>;

// @public
export function updateDoc(reference: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): Promise<void>;
Expand All @@ -477,13 +498,18 @@ export function where(fieldPath: string | FieldPath, opStr: WhereFilterOp, value
// @public
export type WhereFilterOp = '<' | '<=' | '==' | '!=' | '>=' | '>' | 'array-contains' | 'in' | 'array-contains-any' | 'not-in';

// @public
export type WithFieldValue<T> = T extends Primitive ? T : T extends {} ? {
[K in keyof T]: WithFieldValue<T[K]> | FieldValue;
} : Partial<T>;

// @public
export class WriteBatch {
commit(): Promise<void>;
delete(documentRef: DocumentReference<unknown>): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: T): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: Partial<T>, options: SetOptions): WriteBatch;
update(documentRef: DocumentReference<unknown>, data: UpdateData): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: WithFieldValue<T>): WriteBatch;
set<T>(documentRef: DocumentReference<T>, data: NestedPartial<T>, options: SetOptions): WriteBatch;
update<T>(documentRef: DocumentReference<T>, data: UpdateData<T>): WriteBatch;
update(documentRef: DocumentReference<unknown>, field: string | FieldPath, value: unknown, ...moreFieldsAndValues: unknown[]): WriteBatch;
}

Expand Down
10 changes: 7 additions & 3 deletions packages/firebase/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7852,9 +7852,13 @@ declare namespace firebase.storage {
* @param port - The emulator port (ex: 5001)
* @param options.mockUserToken the mock auth token to use for unit testing Security Rules
*/
useEmulator(host: string, port: number, options?: {
mockUserToken?: EmulatorMockTokenOptions | string;
}): void;
useEmulator(
host: string,
port: number,
options?: {
mockUserToken?: EmulatorMockTokenOptions | string;
}
): void;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/compat/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { _FirebaseNamespace } from '@firebase/app-types/private';
import { Component, ComponentType } from '@firebase/component';

import {
FirebaseFirestore,
Firestore as FirebaseFirestore,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Not sure how this worked in the past, since FirebaseFirestore isn't exported by ../exp/index.

CACHE_SIZE_UNLIMITED,
GeoPoint,
Timestamp
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/exp/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export {
SetOptions,
DocumentData,
UpdateData,
Primitive,
WithFieldValue,
NestedUpdateFields,
AddPrefixToKeys,
NestedPartial,
UnionToIntersection,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: I think it would be cleaner if these were exports from a dedicated module, which would make it easier to understand that SetOptions and Primitive are completely unrelated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying to move the helper types into a separate exp/helper.ts file, or for lite as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like lite/types.ts that you can refer to from exp/ and lite/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into lite/types

refEqual,
queryEqual
} from '../src/exp/reference';
Expand Down
8 changes: 7 additions & 1 deletion packages/firestore/lite/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,15 @@ export {
} from '../src/lite/database';

export {
SetOptions,
DocumentData,
UpdateData,
Primitive,
WithFieldValue,
NestedUpdateFields,
AddPrefixToKeys,
NestedPartial,
UnionToIntersection,
SetOptions,
DocumentReference,
Query,
CollectionReference,
Expand Down
25 changes: 15 additions & 10 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ import {
AbstractUserDataWriter
} from '../../exp/index'; // import from the exp public API
import { DatabaseId } from '../core/database_info';
import { NestedPartial, WithFieldValue } from '../lite/reference';
import { UntypedFirestoreDataConverter } from '../lite/user_data_reader';
import { DocumentKey } from '../model/document_key';
import { FieldPath, ResourcePath } from '../model/path';
Expand Down Expand Up @@ -452,9 +453,9 @@ export class Transaction implements PublicTransaction, Compat<ExpTransaction> {
const ref = castReference(documentRef);
if (options) {
validateSetOptions('Transaction.set', options);
this._delegate.set(ref, data, options);
this._delegate.set(ref, data as NestedPartial<T>, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since the method parameter types are T | Partial<T>, we have to cast.

} else {
this._delegate.set(ref, data);
this._delegate.set(ref, data as WithFieldValue<T>);
}
return this;
}
Expand Down Expand Up @@ -513,9 +514,9 @@ export class WriteBatch implements PublicWriteBatch, Compat<ExpWriteBatch> {
const ref = castReference(documentRef);
if (options) {
validateSetOptions('WriteBatch.set', options);
this._delegate.set(ref, data, options);
this._delegate.set(ref, data as NestedPartial<T>, options);
} else {
this._delegate.set(ref, data);
this._delegate.set(ref, data as WithFieldValue<T>);
}
return this;
}
Expand Down Expand Up @@ -597,19 +598,19 @@ class FirestoreDataConverter<U>
);
}

toFirestore(modelObject: U): PublicDocumentData;
toFirestore(modelObject: WithFieldValue<U>): PublicDocumentData;
toFirestore(
modelObject: Partial<U>,
modelObject: NestedPartial<U>,
options: PublicSetOptions
): PublicDocumentData;
toFirestore(
modelObject: U | Partial<U>,
modelObject: WithFieldValue<U> | NestedPartial<U>,
options?: PublicSetOptions
): PublicDocumentData {
if (!options) {
return this._delegate.toFirestore(modelObject as U);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithFieldValue?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._delegate's toFirestore methods use the types on PublicFirestoreDataConverter, which have the old types.

} else {
return this._delegate.toFirestore(modelObject, options);
return this._delegate.toFirestore(modelObject as Partial<U>, options);
}
}

Expand Down Expand Up @@ -733,7 +734,11 @@ export class DocumentReference<T = PublicDocumentData>
set(value: T | Partial<T>, options?: PublicSetOptions): Promise<void> {
options = validateSetOptions('DocumentReference.set', options);
try {
return setDoc(this._delegate, value, options);
if (options) {
return setDoc(this._delegate, value as NestedPartial<T>, options);
} else {
return setDoc(this._delegate, value as WithFieldValue<T>);
}
} catch (e) {
throw replaceFunctionName(e, 'setDoc()', 'DocumentReference.set()');
}
Expand Down Expand Up @@ -1287,7 +1292,7 @@ export class CollectionReference<T = PublicDocumentData>
}

add(data: T): Promise<DocumentReference<T>> {
return addDoc(this._delegate, data).then(
return addDoc(this._delegate, data as WithFieldValue<T>).then(
docRef => new DocumentReference(this.firestore, docRef)
);
}
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/exp/reference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,11 @@ export {
SetOptions,
DocumentData,
UpdateData,
Primitive,
WithFieldValue,
NestedUpdateFields,
AddPrefixToKeys,
NestedPartial,
UnionToIntersection,
refEqual
} from '../lite/reference';
Loading