Skip to content

Add setDoc() #3139

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 61 commits into from
Jun 5, 2020
Merged

Add setDoc() #3139

merged 61 commits into from
Jun 5, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented May 29, 2020

Also adds a lot of tests for existing code and a shared FieldValue class to be used by all SDK versions (comments inline that explain it).

Copy link
Contributor

@rafikhan rafikhan left a comment

Choose a reason for hiding this comment

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

Minor comment on readability. Otherwise LGTM.

Comment on lines 329 to 333
const parsed = isMerge(options)
? dataReader.parseMergeData('setDoc', convertedValue)
: isMergeFields(options)
? dataReader.parseMergeData('setDoc', convertedValue, options.mergeFields)
: dataReader.parseSetData('setDoc', convertedValue);
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 like the helper methods you made to interrogate the options but the ternary required effort to parse. Consider factoring this out into a helper function that includes the toMutations() call or perhaps convert this to a series of if/then/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been bothered by this code for a while, as it is repeated in a couple of places in the SDK. I changed UserDataReader.parseSetData() to perform this check on behalf of the caller.

@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/deletedoc to master June 5, 2020 00:44
@schmidt-sebastian schmidt-sebastian merged commit 2c66c4e into master Jun 5, 2020
@firebase firebase locked and limited conversation to collaborators Jul 6, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/setdoc branch November 9, 2020 22:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants