Skip to content

Add set() overrides to lite sdk #3291

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.

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 23 commits into from
Jun 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .changeset/cyan-books-melt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
---

10 changes: 6 additions & 4 deletions packages/firestore/exp/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ export function setLogLevel(logLevel: LogLevel): void;

export interface FirestoreDataConverter<T> {
toFirestore(modelObject: T): DocumentData;
fromFirestore(snapshot: QueryDocumentSnapshot): T;
toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
fromFirestore(snapshot: QueryDocumentSnapshot<DocumentData>): T;
}

export class FirebaseFirestore {
Expand Down Expand Up @@ -217,9 +218,10 @@ export class WriteBatch {
commit(): Promise<void>;
}

export type SetOptions =
| { merge: true }
| { mergeFields: Array<string | FieldPath> };
export interface SetOptions {
readonly merge?: boolean;
readonly mergeFields?: Array<string | FieldPath>;
}

export class DocumentReference<T = DocumentData> {
private constructor();
Expand Down
10 changes: 6 additions & 4 deletions packages/firestore/lite/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ export function setLogLevel(logLevel: LogLevel): void;

export interface FirestoreDataConverter<T> {
toFirestore(modelObject: T): DocumentData;
fromFirestore(snapshot: QueryDocumentSnapshot): T;
toFirestore(modelObject: Partial<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.

Note that this change also need to be made in exp/index.d.ts

Copy link
Author

Choose a reason for hiding this comment

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

done for converter and SetOptions.

fromFirestore(snapshot: QueryDocumentSnapshot<DocumentData>): T;
}

export class FirebaseFirestore {
Expand Down Expand Up @@ -182,9 +183,10 @@ export class WriteBatch {
commit(): Promise<void>;
}

export type SetOptions =
| { merge: true }
| { mergeFields: Array<string | FieldPath> };
export interface SetOptions {
readonly merge?: boolean;
readonly mergeFields?: Array<string | FieldPath>;
}

export class DocumentReference<T = DocumentData> {
private constructor();
Expand Down
6 changes: 5 additions & 1 deletion packages/firestore/lite/src/api/reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,11 @@ export function setDoc<T>(
): Promise<void> {
const ref = cast(reference, DocumentReference);

const convertedValue = applyFirestoreDataConverter(ref._converter, data);
const convertedValue = applyFirestoreDataConverter(
ref._converter,
data,
options
);
const dataReader = newUserDataReader(ref.firestore);
const parsed = dataReader.parseSetData(
'setDoc',
Expand Down
6 changes: 5 additions & 1 deletion packages/firestore/lite/src/api/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ export class Transaction implements firestore.Transaction {
options?: firestore.SetOptions
): Transaction {
const ref = validateReference(documentRef, this._firestore);
const convertedValue = applyFirestoreDataConverter(ref._converter, value);
const convertedValue = applyFirestoreDataConverter(
ref._converter,
value,
options
);
const parsed = this._dataReader.parseSetData(
'Transaction.set',
ref._key,
Expand Down
7 changes: 5 additions & 2 deletions packages/firestore/lite/src/api/write_batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,11 @@ export class WriteBatch implements firestore.WriteBatch {
this.verifyNotCommitted();
const ref = validateReference(documentRef, this._firestore);

const convertedValue = applyFirestoreDataConverter(ref._converter, value);

const convertedValue = applyFirestoreDataConverter(
ref._converter,
value,
options
);
const parsed = this._dataReader.parseSetData(
'WriteBatch.set',
ref._key,
Expand Down
48 changes: 48 additions & 0 deletions packages/firestore/lite/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
DEFAULT_SETTINGS
} from '../../test/integration/util/settings';
import { AutoId } from '../../src/util/misc';
import { expect } from 'chai';

let appCount = 0;

Expand Down Expand Up @@ -89,3 +90,50 @@ export function withTestCollection(
return fn(collection(db, AutoId.newId()));
});
}

// Used for testing the FirestoreDataConverter.
export class Post {
constructor(readonly title: string, readonly author: string) {}
byline(): string {
return this.title + ', by ' + this.author;
}
}

export const postConverter = {
toFirestore(post: Post): firestore.DocumentData {
return { title: post.title, author: post.author };
},
fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post {
const data = snapshot.data();
return new Post(data.title, data.author);
}
};

export const postConverterMerge = {
toFirestore(
post: Partial<Post>,
options?: firestore.SetOptions
): firestore.DocumentData {
if (
options &&
((options as { merge: true }).merge ||
(options as { mergeFields: Array<string | number> }).mergeFields)
) {
expect(post).to.not.be.an.instanceof(Post);
} else {
expect(post).to.be.an.instanceof(Post);
}
const result: firestore.DocumentData = {};
if (post.title) {
result.title = post.title;
}
if (post.author) {
result.author = post.author;
}
return result;
},
fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post {
const data = snapshot.data();
return new Post(data.title, data.author);
}
};
65 changes: 46 additions & 19 deletions packages/firestore/lite/test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ import {
terminate
} from '../src/api/database';
import {
Post,
postConverter,
postConverterMerge,
withTestCollection,
withTestCollectionAndInitialData,
withTestDb,
Expand Down Expand Up @@ -480,6 +483,30 @@ function genericMutationTests(
});
});

it('supports partials with merge', async () => {
return withTestDb(async db => {
const coll = collection(db, 'posts');
const ref = doc(coll, 'post').withConverter(postConverterMerge);
await setDoc(ref, new Post('walnut', 'author'));
await setDoc(ref, { title: 'olive' }, { merge: true });
const postDoc = await getDoc(ref);
expect(postDoc.get('title')).to.equal('olive');
expect(postDoc.get('author')).to.equal('author');
});
});

it('supports partials with mergeFields', async () => {
return withTestDb(async db => {
const coll = collection(db, 'posts');
const ref = doc(coll, 'post').withConverter(postConverterMerge);
await setDoc(ref, new Post('walnut', 'author'));
await setDoc(ref, { title: 'olive' }, { mergeFields: ['title'] });
const postDoc = await getDoc(ref);
expect(postDoc.get('title')).to.equal('olive');
expect(postDoc.get('author')).to.equal('author');
});
});

it('throws when user input fails validation', () => {
return withTestDoc(async docRef => {
if (validationUsesPromises) {
Expand Down Expand Up @@ -881,7 +908,8 @@ describe('equality', () => {
expect(refEqual(coll1a, coll2)).to.be.false;

const coll1c = collection(firestore, 'a').withConverter({
toFirestore: data => data as firestore.DocumentData,
toFirestore: (data: firestore.DocumentData) =>
data as firestore.DocumentData,
fromFirestore: snap => snap.data()
});
expect(refEqual(coll1a, coll1c)).to.be.false;
Expand All @@ -900,7 +928,8 @@ describe('equality', () => {
expect(refEqual(doc1a, doc2)).to.be.false;

const doc1c = collection(firestore, 'a').withConverter({
toFirestore: data => data as firestore.DocumentData,
toFirestore: (data: firestore.DocumentData) =>
data as firestore.DocumentData,
fromFirestore: snap => snap.data()
});
expect(refEqual(doc1a, doc1c)).to.be.false;
Expand Down Expand Up @@ -968,23 +997,6 @@ describe('equality', () => {
});

describe('withConverter() support', () => {
class Post {
constructor(readonly title: string, readonly author: string) {}
byline(): string {
return this.title + ', by ' + this.author;
}
}

const postConverter = {
toFirestore(post: Post): firestore.DocumentData {
return { title: post.title, author: post.author };
},
fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post {
const data = snapshot.data();
return new Post(data.title, data.author);
}
};

it('for DocumentReference.withConverter()', () => {
return withTestDoc(async docRef => {
docRef = docRef.withConverter(postConverter);
Expand Down Expand Up @@ -1045,4 +1057,19 @@ describe('withConverter() support', () => {
expect(refEqual(docRef, docRef2)).to.be.false;
});
});

it('requires the correct converter for Partial usage', async () => {
return withTestDb(async db => {
const coll = collection(db, 'posts');
const ref = doc(coll, 'post').withConverter(postConverter);
const batch = writeBatch(db);
expect(() =>
batch.set(ref, { title: 'olive' }, { merge: true })
).to.throw(
'Function WriteBatch.set() called with invalid data ' +
'(via `toFirestore()`). Unsupported field value: undefined ' +
'(found in field author in document posts/post)'
);
});
});
});
4 changes: 4 additions & 0 deletions packages/firestore/src/api/user_data_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ const RESERVED_FIELD_REGEX = /^__.*__$/;
*/
export interface UntypedFirestoreDataConverter<T> {
toFirestore(modelObject: T): firestore.DocumentData;
toFirestore(
modelObject: Partial<T>,
options: firestore.SetOptions
): firestore.DocumentData;
fromFirestore(snapshot: unknown, options?: unknown): T;
}

Expand Down