Add settings() method #257

schmidt-sebastian · 2018-07-12T19:04:35Z

This adds a Firestore.settings() method similar to what the Web SDK has. The settings provided in this method will be merged with the settings provided at construction time. The settings() method can only be called once before any other operations.

With this PR, it will be possible to enable timestampsInSnapshots without any further API changes.

codecov · 2018-07-12T19:08:57Z

Codecov Report

Merging #257 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #257   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2119   2137   +18     
  Branches      458    461    +3     
=====================================
+ Hits         2119   2137   +18

Impacted Files	Coverage Δ
src/index.js	`100% <100%> (ø)`	⬆️
src/validate.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b853af...60d0a8c. Read the comment docs.

schmidt-sebastian · 2018-07-12T20:04:49Z

Assigning this to @hiranya911 via this very comment.

hiranya911

Mostly good. Just a few nits.

src/index.js

     * @type {Object}
     */
-    this._initalizationOptions = options;
+    this._initalizationOptions = settings;


src/index.js

+   * Specifies custom settings to be used to configure the `Firestore`
+   * instance. Can only be invoked once and before any other Firestore method.
+   *
+   * If settings are provided via both `settings()` and the Firestore


src/index.js

+    validate.isOptionalBoolean(
+        'settings.timestampsInSnapshots', settings.timestampsInSnapshots);
+
+    if (this._clientInitialized) {


test/index.js

+    });
+  });
+
+  it('validates project ID is string', function() {


schmidt-sebastian

Thanks for the review. Feedback addressed.

src/index.js

     * @type {Object}
     */
-    this._initalizationOptions = options;
+    this._initalizationOptions = settings;


src/index.js

+   * Specifies custom settings to be used to configure the `Firestore`
+   * instance. Can only be invoked once and before any other Firestore method.
+   *
+   * If settings are provided via both `settings()` and the Firestore


src/index.js

+    validate.isOptionalBoolean(
+        'settings.timestampsInSnapshots', settings.timestampsInSnapshots);
+
+    if (this._clientInitialized) {


test/index.js

+    });
+  });
+
+  it('validates project ID is string', function() {


hiranya911

LGTM 👍

schmidt-sebastian · 2018-07-13T16:26:58Z

FYI: The tests that validate the handling of invalid constructor parameters ended up causing unhandled errors in the Node process, as the GCLOUD libraries throw an unhandled error when no credentials are present. I tried to work around this in the tests, but eventually just provided the "fake-credentials.json" as a GOOGLE_APPLICATION_DEFAULT_CREDENTIAL via the package.json.

schmidt-sebastian · 2018-07-13T16:30:24Z

Adding @mikelehen for OWNERS.

schmidt-sebastian added 4 commits July 11, 2018 08:41

Adding TypeScript dev dependencies

d30dfd2

Allowing tests to be run from /src

b72259e

Merge

d24cbca

Test initialization refactor

f4cb701

ghost assigned schmidt-sebastian Jul 12, 2018

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2018

schmidt-sebastian force-pushed the settings branch from d2f6f32 to dd08813 Compare July 12, 2018 19:04

Add settings() method

39b0073

schmidt-sebastian force-pushed the settings branch from dd08813 to 39b0073 Compare July 12, 2018 19:08

hiranya911 suggested changes Jul 12, 2018

View reviewed changes

Review comments

56ad597

schmidt-sebastian force-pushed the settings branch from 531eb06 to 56ad597 Compare July 12, 2018 21:45

schmidt-sebastian commented Jul 12, 2018

View reviewed changes

hiranya911 approved these changes Jul 12, 2018

View reviewed changes

schmidt-sebastian changed the base branch from initrefactor to master July 13, 2018 00:39

schmidt-sebastian force-pushed the settings branch from 30c1809 to e9f798a Compare July 13, 2018 00:40

Master merge

b7dbce5

schmidt-sebastian force-pushed the settings branch from e9f798a to b7dbce5 Compare July 13, 2018 00:41

Merge

519fe0e

schmidt-sebastian force-pushed the settings branch 7 times, most recently from 8261cd1 to 1820e50 Compare July 13, 2018 16:24

schmidt-sebastian force-pushed the settings branch from 1820e50 to 8fe88ee Compare July 13, 2018 16:27

Fixing default credentials error

60d0a8c

schmidt-sebastian force-pushed the settings branch from 8fe88ee to 60d0a8c Compare July 13, 2018 16:29

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jul 13, 2018

mikelehen approved these changes Jul 13, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 13, 2018

schmidt-sebastian merged commit c83e047 into master Jul 13, 2018

schmidt-sebastian mentioned this pull request Jul 13, 2018

Removing assert that fails the system-tests on CI #260

Merged

schmidt-sebastian deleted the settings branch July 24, 2018 04:26

Add settings() method #257

Add settings() method #257

Uh oh!

Conversation

schmidt-sebastian commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schmidt-sebastian commented Jul 12, 2018

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Jul 13, 2018

Uh oh!

schmidt-sebastian commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

schmidt-sebastian commented Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading