Finish TypeScript Migration #512

schmidt-sebastian · 2018-12-29T00:32:53Z

This PR finishes the Typescript migration of the Firestore Server SDK. As part of this:

I changed the Validator class to use free-standing functions. This gets rid of the dynamic method creation, which doesn't really work with strong typing
I changed the client to use unknown instead of any for the user-facing types (only internally, not in the types we shipped)
I brought the client more in line with the Web SDK and only support the direction/query operator spelling that is outlined in the type definition (for example, "ASC" no longer works). This lets me use a common property validation function (and is the only breaking change)
I removed the dependency on is to be able to use TypeScript's typechecking.

This is the PR that I am planning on merging, and it's also the PR that I am would like to address all feedback in (to reduce merge conflicts). Since it is not very easy to review, I split this out into a series of PRs that are more tightly scoped:

~~Cleanup: Argument count validation #495 Cleanup: Argument count validation~~ (approved)
~~Cleanup: Validate primitive types #496 Cleanup: Validate primitive types~~ (approved)
~~Cleanup: Move parseGetAllArguments #497 Cleanup: Move parseGetAllArguments~~ (approved)
Cleanup: Enforce Import ordering #498 Cleanup: Enforce Import ordering
Cleanup: Remove validator #499 Cleanup: Remove validator
Cleanup: Remove 'any' #500 Cleanup: Remove 'any'
~~Cleanup: Remove is #503 Cleanup: Remove is~~ (approved)
~~refactor: Add strict types for the transaction test #504 Cleanup: Add strict types for the transaction test~~ (approved)
~~Cleanup: Field value validation #505 Cleanup: Field value validation~~ (approved)
~~Cleanup: Misc #506 Cleanup: Misc~~ (approved)
~~Cleanup: Error messages for custom objects #507 Cleanup: Error messages for custom objects~~ (approved)
Cleanup: Enum validation #508 Cleanup: Enum validation (<-- this is the breaking change)
549d90e Moves the field value validation to the serializer file

The individual PRs are not meant to compile/pass tests. There should not be anything in this larger PR that is not listed above.

This is fairly low priority, so please take your time to review.

Fixes: b/117464450 and b/119350730

schmidt-sebastian · 2018-12-29T00:33:36Z

@JustinBeckwith / Node team:

This PR fails CI with the following errors (re unknown[] usage):

/Users/mrschmidt/GitHub/googleapis/nodejs-firestore/dev/src/serializer.ts Array type using 'T[]' is forbidden for non-simple types. Use 'Array<T>' instead. (array-type)

Note that gts check passes (gts --version reports 0.8.0, even though 0.9.0 is installed), but npm run gts check -- as run by the CI -- does not (npm rum gts --version reports 6.2.0). gts fix actually converts Array<unknown> to unknown[].

mikelehen

Sorry for the slight delay. I looked over the individual PRs and left a few minor comments. I think mostly this looks good but we need to fix the merge conflicts and revert the breaking changes (sorry).

mikelehen · 2019-02-21T17:13:22Z

dev/src/write-batch.ts

+    throw new Error(`${
+        invalidArgumentMessage(
+            arg, 'precondition')} Input contains more than one condition.`);
+  }


Should this be checking for invalid preconditions? E.g. if I typo and do {exist: true} instead of {exists: true}

I filed b/125363185 to tackle this at a later point.

mikelehen · 2019-02-21T17:13:55Z

dev/src/write-batch.ts

+ * @private
+ * @param arg The argument name or argument index (for varargs methods).
+ * @param value The object to validate.
+ * @param options Optional validation options specifying whether the valued can


"valued" => "value" here and elsewhere.

mikelehen · 2019-02-21T17:17:54Z

dev/src/validate.ts

 *
 * @private
 */
-function formatPlural(num, str) {
-  return `${num} ${str}` + (num === 1 ? '' : 's');
+export interface AllowOptional {


nit: I don't love AllowOptional and AllowRange as names. Normally I like to name classes / interfaces as nouns. I think maybe OptionalOption and RangeOptions would be better, though admittedly OptionalOption is a bit awkward. :)

Renamed to RequiredArgumentOptions and NumericRangeOptions. Not sure if better but it is different!

mikelehen · 2019-02-21T17:18:54Z

dev/src/write-batch.ts

+ * @param allowExist Whether to allow the 'exists' preconditions.
+ */
+function validatePrecondition(
+    arg: string|number, value: unknown, allowExist: boolean): void {


allowExist => allowExists ? (you use "/allowExists=/" elsewhere)

mikelehen · 2019-02-21T17:20:50Z

dev/src/write-batch.ts

+  if (conditions > 1) {
+    throw new Error(`${
+        invalidArgumentMessage(
+            arg, 'precondition')} Input contains more than one condition.`);


This error and above use "condition" instead of "precondition". Is that intentional? We should probably use whatever leads to the most consistency across our APIs / docs / errors.

It was, and the argument for that is that a precondition can only contain one condition. I can also see that someone might treat { exists: true } and { lastUpdateTime: Date.now() } as single preconditions, in which case { exists: true, lastUpdateTime: Date.now() } would make two preconditions. I don't have a strong preference here, but since I managed to confuse you, let's err on the side of less confusion and use the term precondition.

mikelehen · 2019-02-21T17:29:05Z

dev/src/write-batch.ts

+ * @param value The object to validate.
+ * @param options Optional validation options specifying whether the valued can
+ * be omitted.
+ * @returns 'true' if the input is a valid SetOptions object.


Should be @throws .... I think several other methods are wrong too.

Fixed, but I only found this one instance. There are still some internal helpers that do return a boolean.

mikelehen · 2019-02-21T17:32:15Z

dev/test/document.ts

@@ -957,29 +958,29 @@ describe('set document', () => {
      firestore.doc('collectionId/documentId').set({foo: 'bar'}, 'foo');
    })
        .to.throw(
-            'Argument "options" is not a valid SetOptions. Input is not an object.');
+            'Argument "options" is not a valid set() option. Input is not an object.');


This reads a bit weird. Maybe "is not a valid set() options object." ? though admittedly that's a bit verbose.

Changed to "is not a valid set() options argument".

mikelehen · 2019-02-21T17:36:17Z

dev/src/document.ts

@@ -349,7 +348,7 @@ export class DocumentSnapshot {
   *   console.log(`Retrieved data: ${JSON.stringify(data)}`);
   * });
   */
-  data(): DocumentData|undefined {
+  data(): {[field: string]: any}|undefined {  // tslint:disable-line no-any


Are all of these remaining instances of "any" expected to stay forever? Or do you intend further cleanup passes? In my ideal world we'd either remove them or comment them. :-) This one may be intentional so that our users don't have to do instanceof checks on the data they get back? I'm not sure though.

Yes, I think that we do need to use any here. We don't have a way to control what we are giving back, and by using any as a return type we indicate that the user has to do some type checking of their own. I added a comment.

by using any as a return type we indicate that the user has to do some type checking of their own

Sorry for the nit, but that's not actually what any does. 😀 any makes it so you don't have to do any type checking (you can do anything you want with it and TypeScript won't complain). If we want to force the user to do type-checking, then we should return unknown (new in TS 3.x) instead, which you can't do anything with unless you first do type-checking on it.

That said, using any here is consistent with the mobile SDK (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts#L24), so I'm fine with this.

Uhm, that's what I meant (kind of, but not really).

Changed to

// We deliberately use `any` in the external API to not impose type-checking // on end users.

mikelehen · 2019-02-21T17:41:27Z

dev/src/reference.ts

-
-  return true;
+export function validateQueryOrder(arg: string|number, op: unknown): void {
+  validateEnumValue(arg, op, Object.keys(directionOperators), {optional: true});


Per discussions with Gil, etc. during the push to GA, I think we want to unroll the breaking change we made here (and elsewhere). And in fact we may want to relax the mobile SDK to match.

I brought back the old behavior by pre-preprocessing op.

mikelehen · 2019-02-21T17:43:37Z

dev/src/reference.ts

@@ -60,7 +58,6 @@ const comparisonOperators:
    {[k: string]: api.StructuredQuery.FieldFilter.Operator} = {
      '<': 'LESS_THAN',
      '<=': 'LESS_THAN_OR_EQUAL',
-      '=': 'EQUAL',
      '==': 'EQUAL',


Per discussions with Gil, etc. during the push to GA, I think we want to continue to allow '=' to avoid the breaking change.

I changed the validation function to accept both = and ==. The function returns the sanitized value.

codecov · 2019-02-21T22:31:46Z

Codecov Report

Merging #512 into master will decrease coverage by 0.08%.
The diff coverage is 96.98%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   95.93%   95.85%   -0.09%     
==========================================
  Files          24       24              
  Lines        1943     1928      -15     
  Branches      161      166       +5     
==========================================
- Hits         1864     1848      -16     
  Misses         57       57              
- Partials       22       23       +1

Impacted Files	Coverage Δ
dev/src/types.ts	`100% <ø> (ø)`	⬆️
dev/src/document-change.ts	`100% <ø> (ø)`	⬆️
dev/src/timestamp.ts	`100% <100%> (ø)`	⬆️
dev/src/watch.ts	`99.04% <100%> (ø)`	⬆️
dev/src/order.ts	`95.4% <100%> (-0.06%)`	⬇️
dev/src/field-value.ts	`93.75% <100%> (+1.29%)`	⬆️
dev/src/logger.ts	`100% <100%> (ø)`	⬆️
dev/src/geo-point.ts	`91.66% <100%> (-0.65%)`	⬇️
dev/src/write-batch.ts	`100% <100%> (ø)`	⬆️
dev/src/index.ts	`98.17% <100%> (+0.96%)`	⬆️
... and 8 more

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 3f960ca...f681862. Read the comment docs.

codecov · 2019-02-21T22:31:47Z

Codecov Report

Merging #512 into master will decrease coverage by 0.07%.
The diff coverage is 97.03%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   95.93%   95.85%   -0.08%     
==========================================
  Files          24       24              
  Lines        1943     1932      -11     
  Branches      161      168       +7     
==========================================
- Hits         1864     1852      -12     
  Misses         57       57              
- Partials       22       23       +1

Impacted Files	Coverage Δ
dev/src/types.ts	`100% <ø> (ø)`	⬆️
dev/src/document-change.ts	`100% <ø> (ø)`	⬆️
dev/src/timestamp.ts	`100% <100%> (ø)`	⬆️
dev/src/watch.ts	`99.04% <100%> (ø)`	⬆️
dev/src/order.ts	`95.4% <100%> (-0.06%)`	⬇️
dev/src/field-value.ts	`93.75% <100%> (+1.29%)`	⬆️
dev/src/logger.ts	`100% <100%> (ø)`	⬆️
dev/src/geo-point.ts	`91.66% <100%> (-0.65%)`	⬇️
dev/src/write-batch.ts	`100% <100%> (ø)`	⬆️
dev/src/index.ts	`98.17% <100%> (+0.96%)`	⬆️
... and 8 more

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 6b5bd1e...4515566. Read the comment docs.

mikelehen

LGTM with nits.

mikelehen · 2019-02-22T00:03:38Z

dev/src/document.ts

@@ -349,7 +348,7 @@ export class DocumentSnapshot {
   *   console.log(`Retrieved data: ${JSON.stringify(data)}`);
   * });
   */
-  data(): DocumentData|undefined {
+  data(): {[field: string]: any}|undefined {  // tslint:disable-line no-any


by using any as a return type we indicate that the user has to do some type checking of their own

Sorry for the nit, but that's not actually what any does. 😀 any makes it so you don't have to do any type checking (you can do anything you want with it and TypeScript won't complain). If we want to force the user to do type-checking, then we should return unknown (new in TS 3.x) instead, which you can't do anything with unless you first do type-checking on it.

That said, using any here is consistent with the mobile SDK (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts#L24), so I'm fine with this.

mikelehen · 2019-02-22T00:08:17Z

dev/src/reference.ts

@@ -785,6 +786,8 @@ export class QuerySnapshot {
   *   });
   * });
   */
+  // We deliberately use `any` to allow users to use arbitrary objects as the
+  // callback's context.


I think you can use unknown here since we don't care what the type is, we just pass it through.

Yes, that works. Not sure how .call() is defined, but the linter doesn't complain.

mikelehen · 2019-02-22T00:13:08Z

dev/src/validate.ts

@@ -165,7 +169,7 @@ export function validateBoolean(
 */
 export function validateNumber(
    arg: string|number, value: unknown,
-    options?: AllowOptional&AllowRange): void {
+    options?: RequiredArgumentOptions&NumericRangeOptions): void {


Is this really how clang-format formats this? Yuck. 😒

mikelehen · 2019-02-22T00:22:14Z

dev/test/util/helpers.ts

@@ -14,20 +14,22 @@
 * limitations under the License.
 */

+const v1beta1 = require('../../src/v1beta1');


Removed. Bye bye v1beta1

schmidt-sebastian added 27 commits December 28, 2018 12:54

Cleanup: Misc

6f27d84

Cleanup: Error messages for custom objects

cafb0a2

Cleanup: Enum validation

10dc1ab

Remove

f9ace4c

Cleanup: Enforce Import ordering

ff1b0a7

Cleanup: Move parseGetAllArguments

db5bd60

Cleanup: Argument count validation

65bb44d

Cleanup: Validate primitive types

7a0e90a

Merge branch 'mrschmidt-basictypes' into mrschmidt-mergedcleanup

7342f9a

Merge mrschmidt-parsegetall

a1d754c

Merge mrschmidt-imports

1898cda

Cleanup: Remove validator

0b276e4

Merge mrschmidt-novalidator

d456690

Fix typo

5cb4f71

Merge branch 'mrschmidt-novalidator' into mrschmidt-mergedcleanup

717fb16

Merge mrschmidt-removeany

0684fe3

Cleanup: Remove

97ef7bf

Merge mrschmidt-removeis

62d68d7

Cleanup: Add strict types for the transaction test

d07569e

Merge branch 'mrschmidt-transactiontest' into mrschmidt-mergedcleanup

ba2c24d

Cleanup: Field value validation

10f2b50

Merge mrschmidt-validateuserinput

f88837f

Merge mrschmidt-randomnits

42cae40

Merge branch 'mrschmidt-customobjecterror' into mrschmidt-mergedcleanup

9878b7f

Merge mrschmidt-validateenumvalue

395d139

Remove unused methods

6262c67

Compilation fixes

2373f67

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

schmidt-sebastian changed the title ~~Finish TypeScript Migration (BREAKING CHANGE~~ Finish TypeScript Migration (BREAKING CHANGE) Dec 29, 2018

JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019

yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019

JustinBeckwith added the needs work This is a pull request that needs a little love. label Feb 9, 2019

yoshi-automation removed the 🚨 This issue needs some love. label Feb 9, 2019

mikelehen suggested changes Feb 21, 2019

View reviewed changes

schmidt-sebastian added 2 commits February 21, 2019 11:00

Manual merge

c244809

Running gtx fix

d2c50f1

mikelehen assigned schmidt-sebastian and unassigned mikelehen Feb 21, 2019

schmidt-sebastian added 9 commits February 21, 2019 11:25

Add comments for no-any

d92ceb7

Rename AllowOptional and AllowRange

dbc7669

Rename allowExist

2896c7e

Use in error message

bda9467

Typo: valued -> value

d88e8e0

@returns -> @throws

da63660

Change options error message

19d08a1

Add back support for old enum values

1369b7f

Cleanup

f681862

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Feb 21, 2019

schmidt-sebastian changed the title ~~Finish TypeScript Migration (BREAKING CHANGE)~~ Finish TypeScript Migration Feb 21, 2019

Fix lint

9734212

mikelehen approved these changes Feb 22, 2019

View reviewed changes

schmidt-sebastian added 2 commits February 21, 2019 20:06

Comments

0a3381b

Merge branch 'master' into mrschmidt-mergedcleanup

4515566

schmidt-sebastian merged commit b1f6c75 into master Feb 22, 2019

schmidt-sebastian deleted the mrschmidt-mergedcleanup branch March 11, 2019 03:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish TypeScript Migration #512

Finish TypeScript Migration #512

schmidt-sebastian commented Dec 29, 2018 •

edited

schmidt-sebastian commented Dec 29, 2018 •

edited

mikelehen left a comment

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 22, 2019

schmidt-sebastian Feb 22, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

mikelehen Feb 21, 2019

schmidt-sebastian Feb 21, 2019

codecov bot commented Feb 21, 2019

codecov bot commented Feb 21, 2019 •

edited

mikelehen left a comment

mikelehen Feb 22, 2019

mikelehen Feb 22, 2019

schmidt-sebastian Feb 22, 2019

mikelehen Feb 22, 2019

mikelehen Feb 22, 2019

schmidt-sebastian Feb 22, 2019

Finish TypeScript Migration #512

Finish TypeScript Migration #512

Conversation

schmidt-sebastian commented Dec 29, 2018 • edited

schmidt-sebastian commented Dec 29, 2018 • edited

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2019

Codecov Report

codecov bot commented Feb 21, 2019 • edited

Codecov Report

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Dec 29, 2018 •

edited

schmidt-sebastian commented Dec 29, 2018 •

edited

codecov bot commented Feb 21, 2019 •

edited