Skip to content
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

fix: make SetOptions a union type #1618

Merged
merged 2 commits into from
Oct 14, 2021
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
24 changes: 3 additions & 21 deletions dev/src/write-batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,8 @@ export class WriteBatch implements firestore.WriteBatch {
options?: firestore.SetOptions
): WriteBatch {
validateSetOptions('options', options, {optional: true});
const mergeLeaves = options && options.merge === true;
const mergePaths = options && options.mergeFields;
const mergeLeaves = options && 'merge' in options && options.merge;
const mergePaths = options && 'mergeFields' in options;
const ref = validateDocumentReference('documentRef', documentRef);
let firestoreData: firestore.DocumentData;
if (mergeLeaves || mergePaths) {
Expand Down Expand Up @@ -763,27 +763,9 @@ export function validateSetOptions(
);
}

const setOptions = value as {[k: string]: unknown};

if ('merge' in setOptions && typeof setOptions.merge !== 'boolean') {
throw new Error(
`${invalidArgumentMessage(
arg,
'set() options argument'
)} "merge" is not a boolean.`
);
}
const setOptions = value as {mergeFields: Array<string | FieldPath>};

if ('mergeFields' in setOptions) {
if (!Array.isArray(setOptions.mergeFields)) {
throw new Error(
`${invalidArgumentMessage(
arg,
'set() options argument'
)} "mergeFields" is not an array.`
);
}

for (let i = 0; i < setOptions.mergeFields.length; ++i) {
try {
validateFieldPath(i, setOptions.mergeFields[i]);
Expand Down
4 changes: 2 additions & 2 deletions dev/system-test/firestore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3113,7 +3113,7 @@ describe('Types test', () => {
testObj: PartialWithFieldValue<TestObject>,
options?: SetOptions
) {
if (options && (options.merge || options.mergeFields)) {
if (options) {
expect(testObj).to.not.be.an.instanceOf(TestObject);
} else {
expect(testObj).to.be.an.instanceOf(TestObject);
Expand Down Expand Up @@ -3220,7 +3220,7 @@ describe('Types test', () => {
testObj: PartialWithFieldValue<TestObject>,
options?: SetOptions
) {
if (options && (options.merge || options.mergeFields)) {
if (options) {
expect(testObj).to.not.be.an.instanceOf(TestObject);
} else {
expect(testObj).to.be.an.instanceOf(TestObject);
Expand Down
40 changes: 6 additions & 34 deletions dev/test/document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1308,56 +1308,28 @@ describe('set document', () => {
});

it('validates merge option', () => {
expect(() => {
firestore
.doc('collectionId/documentId')
.set({foo: 'bar'}, 'foo' as InvalidApiUsage);
}).to.throw(
'Value for argument "options" is not a valid set() options argument. Input is not an object.'
);

expect(() => {
firestore.doc('collectionId/documentId').set(
{foo: 'bar'},
{
merge: 42 as InvalidApiUsage,
}
);
}).to.throw(
'Value for argument "options" is not a valid set() options argument. "merge" is not a boolean.'
);

expect(() => {
firestore.doc('collectionId/documentId').set(
{foo: 'bar'},
{
mergeFields: 42 as InvalidApiUsage,
mergeFields: ['foobar'],
}
);
}).to.throw(
'Value for argument "options" is not a valid set() options argument. "mergeFields" is not an array.'
);
}).to.throw('Input data is missing for field "foobar".');

expect(() => {
firestore.doc('collectionId/documentId').set(
{foo: 'bar'},
{
mergeFields: [null as InvalidApiUsage],
mergeFields: ['foobar..'],
}
);
}).to.throw(
'Value for argument "options" is not a valid set() options argument. "mergeFields" is not valid: Element at index 0 is not a valid field path. Paths can only be specified as strings or via a FieldPath object.'
'Value for argument "options" is not a valid set() options argument. ' +
'"mergeFields" is not valid: Element at index 0 is not a valid ' +
'field path. Paths must not contain ".." in them.'
);

expect(() => {
firestore.doc('collectionId/documentId').set(
{foo: 'bar'},
{
mergeFields: ['foobar'],
}
);
}).to.throw('Input data is missing for field "foobar".');

expect(() => {
firestore
.doc('collectionId/documentId')
Expand Down
2 changes: 1 addition & 1 deletion dev/test/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ export const postConverterMerge = {
post: PartialWithFieldValue<Post>,
options?: SetOptions
): DocumentData {
if (options && (options.merge || options.mergeFields)) {
if (options) {
expect(post).to.not.be.an.instanceOf(Post);
} else {
expect(post).to.be.an.instanceof(Post);
Expand Down
33 changes: 15 additions & 18 deletions types/firestore.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1065,25 +1065,22 @@ declare namespace FirebaseFirestore {
* `DocumentReference`, `WriteBatch` and `Transaction`. These calls can be
* configured to perform granular merges instead of overwriting the target
* documents in their entirety.
*
* @param merge Changes the behavior of a set() call to only replace the
* values specified in its data argument. Fields omitted from the set() call
* remain untouched.
*
* @param mergeFields Changes the behavior of set() calls to only replace
* the specified field paths. Any field path that is not specified is ignored
* and remains untouched.
*/
export interface SetOptions {
/**
* Changes the behavior of a set() call to only replace the values specified
* in its data argument. Fields omitted from the set() call remain
* untouched.
*/
readonly merge?: boolean;

/**
* Changes the behavior of set() calls to only replace the specified field
* paths. Any field path that is not specified is ignored and remains
* untouched.
*
* It is an error to pass a SetOptions object to a set() call that is
* missing a value for any of the fields specified here.
*/
readonly mergeFields?: (string | FieldPath)[];
}
export type SetOptions =
| {
readonly merge?: boolean;
}
| {
readonly mergeFields?: Array<string | FieldPath>;
};

/**
* An options object that can be used to configure the behavior of `getAll()`
Expand Down