-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat: add Precondition.exists to delete() #1505
Conversation
dev/src/index.ts
Outdated
@@ -243,6 +243,9 @@ const MAX_CONCURRENT_REQUESTS_PER_CLIENT = 100; | |||
* @property {Timestamp} lastUpdateTime The update time to enforce. If set, | |||
* enforces that the document was last updated at lastUpdateTime. Fails the | |||
* operation if the document was last updated at a different time. | |||
* @property {boolean} exists If set, enforces that the target document must | |||
* exist. This property | |||
* is only valid with delete operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessarily true. It can be used for sets with exists:false. There is also a difference in behavior for true and false and undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those preconditions are added by the SDK, but the developer is not allowed to use exists:false
with set()
, since Preconditions
can only be used with update()
and delete()
for developers. Since this is developer-facing documentation, I think it should be catered to their use case.
I also realized that exists
was not added to the firestore.d.ts
Precondition
interface, so I added that as well.
dev/src/reference.ts
Outdated
@@ -371,6 +371,8 @@ export class DocumentReference<T = firestore.DocumentData> | |||
* @param {Timestamp=} precondition.lastUpdateTime If set, enforces that the | |||
* document was last updated at lastUpdateTime. Fails the delete if the | |||
* document was last updated at a different time. | |||
* @param {boolean=} precondition.exists If set, enforces that the target document | |||
* must exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must or must not exist (or something like that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/index.ts
Outdated
@@ -243,6 +243,8 @@ const MAX_CONCURRENT_REQUESTS_PER_CLIENT = 100; | |||
* @property {Timestamp} lastUpdateTime The update time to enforce. If set, | |||
* enforces that the document was last updated at lastUpdateTime. Fails the | |||
* operation if the document was last updated at a different time. | |||
* @property {boolean} exists If set, enforces that the target document must | |||
* or must not exist. This property is only valid with delete operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete the last sentence. It can be used with delete(), even if it does not make a lot of sense. It could be used to trigger an error if the deleted document does not exist for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Adding
Preconditions.exists()
to documentation and TS autocomplete, as requested in comment.