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

feat!: add QoL improvements for converters #1595

Merged
merged 3 commits into from
Aug 24, 2021
Merged

Conversation

thebrianchen
Copy link

Porting from web.

Fixes #1448.

@thebrianchen thebrianchen self-assigned this Aug 24, 2021
@thebrianchen thebrianchen requested review from a team as code owners August 24, 2021 00:07
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2021
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Aug 24, 2021
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

dev/system-test/firestore.ts Outdated Show resolved Hide resolved
/**
* Update data (for use with [update]{@link DocumentReference#update})
* that contains paths mapped to values. Fields that contain dots reference
* nested fields within the document.
* nested fields within the document. FieldValues can be passed in
* as property values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud: Is "property" the right term and does it make it harder to understand? Should we drop it and just call it "values"?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to specify "property values" to convey that FieldValues can be set as the value of an object property, but not as the entire object value.

@thebrianchen thebrianchen merged commit 39e5138 into bc/4.1 Aug 24, 2021
@thebrianchen thebrianchen deleted the bc/types-revamp branch August 24, 2021 16:49
@Jeandcc
Copy link

Jeandcc commented Nov 2, 2021

Apologies for pinging here, I'm just a bit confused by all the issues that reference this same "need". Has this been published to any version of the "firebase-admin" package, or is it still in the pipeline to be released?

Thanks!

@schmidt-sebastian
Copy link
Contributor

This has not been published. We are still working on some details for the implementation that we published for the @firebase/firestore Web SDK and will publish the Node version once these changes have been finalized.

@oxc
Copy link

oxc commented Jul 5, 2022

I'm a bit confused by the changes implemented for the update() method signature. The update() method does, to the best of my knowledge, not call the converter. Its parameters will not be converted, and must match the "storage format". So it should in fact not be an UpdateData<T>, but an UpdateData<D> where D is the theoretical type that reflects the stored data (not the converted one).

Am I missing something here?

EDIT: I've reported this as #1745

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants