feat!: convert Gapic client to TypeScript #805

alexander-fenster · 2019-12-06T00:58:23Z

Converting the auto-generated parts of the library to TypeScript using the new https://github.com/googleapis/gapic-generator-typescript.

alexander-fenster · 2019-12-06T01:07:09Z

dev/src/v1/firestore_client_config.json

          "total_timeout_millis": 600000
        }
      },
      "methods": {
        "GetDocument": {
          "timeout_millis": 60000,
-          "retry_codes_name": "idempotent",
+          "retry_codes_name": "d_e_a_d_l_i_n_e_e_x_c_e_e_d_e_d_i_n_t_e_r_n_a_l_u_n_a_v_a_i_l_a_b_l_e",


This is a scary auto-generated name, looks like a bug - I'll figure out

googleapis/gapic-generator-typescript#172

alexander-fenster · 2019-12-06T01:08:21Z

dev/protos/firestore_proto_api.d.ts

@@ -1,17 +1,3 @@
-// Copyright 2019 Google LLC


Was the license added manually? I just used the update script.

No. I wonder if it was added later when we migrated to the new Copyright header?

alexander-fenster · 2019-12-06T01:08:40Z

dev/protos/update.sh

@@ -50,6 +50,14 @@ mkdir -p "${PROTOS_DIR}/google/firestore/v1"
 cp googleapis/google/firestore/v1/*.proto \
   "${PROTOS_DIR}/google/firestore/v1/"

+mkdir -p "${PROTOS_DIR}/google/firestore/v1beta1"


Now we need all the protos there.

schmidt-sebastian

YAY! Getting close to .close().

Thank you so much.

schmidt-sebastian · 2019-12-06T05:38:15Z

dev/protos/firestore_proto_api.d.ts

@@ -1,17 +1,3 @@
-// Copyright 2019 Google LLC


No. I wonder if it was added later when we migrated to the new Copyright header?

dev/protos/firestore_proto_api.js

schmidt-sebastian · 2019-12-06T05:42:31Z

dev/protos/protos.json

                  },
                  "nested": {
+                    "Index": {


Again, would the clients load faster if we split Admin/Non-Admin Protos?

schmidt-sebastian · 2019-12-06T05:44:22Z

dev/src/v1/firestore_admin_client.ts

+const version = require('../../../package.json').version;
+
+/**
+ *  Operations are created by service `FirestoreAdmin`, but are accessed via


Nit: Leading spaces.

dev/src/v1/firestore_admin_client_config.json

schmidt-sebastian · 2019-12-06T06:08:04Z

dev/test/gapic-firestore-v1.ts

  it('has apiEndpoint', () => {
    const apiEndpoint = firestoreModule.v1.FirestoreClient.apiEndpoint;
    assert(apiEndpoint);
  });
-


Nit: Keep empty lines (again, don't fix before merging)

schmidt-sebastian · 2019-12-06T06:09:54Z

dev/test/gapic-firestore-v1.ts


-'use strict';


It would be interesting to know if the old system tests pass against the new client. Did you test that by any chance?

schmidt-sebastian · 2019-12-06T06:14:25Z

synth.py


 # Fix dropping of google-cloud-resource-header
 # See: https://github.com/googleapis/nodejs-firestore/pull/375
 s.replace(
-    "dev/src/v1beta1/firestore_client.js",
+    "dev/src/v1beta1/firestore_client.ts",
    "return this\._innerApiCalls\.listen\(options\);",
    "return this._innerApiCalls.listen({}, options);",


Is this still needed (or rather: does the original bug still exist in the TS generated client?). It would seem that this should now be caught by the type system.

tsconfig.json

alexander-fenster

Thanks @schmidt-sebastian - I'll make more changes on Friday :)

We need to also include the field_definition and client protos

codecov · 2019-12-06T22:05:34Z

Codecov Report

Merging #805 into master will increase coverage by 16.12%.
The diff coverage is 66.26%.

@@             Coverage Diff             @@
##           master     #805       +/-   ##
===========================================
+ Coverage   74.24%   90.36%   +16.12%     
===========================================
  Files          50       25       -25     
  Lines        2904     2814       -90     
  Branches      462      707      +245     
===========================================
+ Hits         2156     2543      +387     
+ Misses        695      118      -577     
- Partials       53      153      +100

Impacted Files	Coverage Δ
dev/src/index.ts	`96.96% <ø> (ø)`	⬆️
dev/src/timestamp.ts	`92.59% <ø> (ø)`	⬆️
dev/src/field-value.ts	`95% <ø> (ø)`	⬆️
dev/src/document.ts	`96.15% <ø> (ø)`	⬆️
dev/src/reference.ts	`99.03% <ø> (ø)`	⬆️
dev/src/convert.ts	`92.4% <ø> (ø)`	⬆️
dev/src/order.ts	`95.19% <ø> (ø)`	⬆️
dev/src/write-batch.ts	`98.63% <ø> (ø)`	⬆️
dev/src/watch.ts	`95.98% <ø> (ø)`	⬆️
dev/src/geo-point.ts	`91.66% <ø> (ø)`	⬆️
... and 12 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 a9cab55...19adbc4. Read the comment docs.

schmidt-sebastian · 2019-12-14T02:57:30Z

The system tests should pass once googleapis/gapic-generator-typescript#173 is merged and released.

alexander-fenster · 2019-12-14T04:19:11Z

I'm guessing we first need to update synth.py to make changes for your latest fixes, then run the generator again.

schmidt-sebastian · 2019-12-15T01:46:52Z

Re-ran synthool and pushed changes. The only remaining blockers (in my opinion) are the setting for initial_rpc_timeout_millis and max_rpc_timeout_millis. Should we drop them?

schmidt-sebastian · 2019-12-15T01:55:01Z

Updated all the dependencies since the CI requires a newer version of gts than in package.json and the generated clients require a newer version of Google Gax (for gax.OperationsClient).

alexander-fenster · 2019-12-15T06:41:21Z

Dropping of initial_rpc_timeout_millis and others will be done in gax (the exported type requires those unused parameters to be set). I will first remove them from there, then from the generator. All generated libraries will be updated within the nightly synthtool run.

I think we are good to go here! I suggest we treat this PR as a breaking change just in case (feel free to merge the PR then), otherwise, if you think it's just a minor, remove ! from the PR title before merging :) It's up to you.

Upd. The parameters are actually used but will be dropped soon.

alexander-fenster · 2019-12-15T07:08:10Z

googleapis/gapic-generator-typescript#175 will reset the parameters to 60000. We can merge now (this change will come within a nightly synthtool run) or wait for this change to make its way into the Docker image (will happen on Monday).

willbattel · 2020-06-04T23:21:49Z

This PR is linked in the v3.0.0 release notes as a breaking change. Is there anything that users of this library need to change to stay compliant? It looks mostly internal as far as I can tell.

schmidt-sebastian · 2020-06-05T00:04:54Z

We bumped the major version because of the risk with this PR. We are not aware of any specific breakages.

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

feat!: convert to TypeScript

21f773f

alexander-fenster force-pushed the typescript branch from f6e2c7f to 21f773f Compare December 6, 2019 01:05

alexander-fenster requested a review from schmidt-sebastian December 6, 2019 01:06

alexander-fenster commented Dec 6, 2019

View reviewed changes

schmidt-sebastian reviewed Dec 6, 2019

View reviewed changes

alexander-fenster commented Dec 6, 2019

View reviewed changes

schmidt-sebastian mentioned this pull request Dec 6, 2019

feat!: remove deprecated timestampInSnapshots setting #808

Merged

fix: make conformance test pass (#809)

f01a8d3

We need to also include the field_definition and client protos

alexander-fenster mentioned this pull request Dec 6, 2019

build: publish annotated protos and new retry config #807

Closed

refactor: split proto files (#810)

2f43d5c

bcoe mentioned this pull request Dec 9, 2019

build: moves to new proto annotations #811

Closed

Merge branch 'master' into typescript

790fe98

fix: add license to generated protos (#816)

67ce1e6

alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2019

schmidt-sebastian added 2 commits December 15, 2019 09:31

Use LLC license

8426686

re-run synthtool, update google-gax

2421912

schmidt-sebastian added 2 commits December 15, 2019 09:52

Update all dependencies

0e499aa

Re-run prettier

28e56a4

schmidt-sebastian approved these changes Dec 15, 2019

View reviewed changes

schmidt-sebastian mentioned this pull request Dec 15, 2019

HUGE memory leak problem #768

Closed

rerun synthtool to trigger ci

19adbc4

schmidt-sebastian changed the title ~~feat!: convert to TypeScript~~ feat!: convert Gapic client to TypeScript Dec 15, 2019

schmidt-sebastian merged commit 5000b2d into master Dec 15, 2019

release-please bot mentioned this pull request Dec 15, 2019

chore: release 3.0.0 #815

Merged

schmidt-sebastian deleted the typescript branch January 3, 2020 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: convert Gapic client to TypeScript #805

feat!: convert Gapic client to TypeScript #805

alexander-fenster commented Dec 6, 2019 •

edited

Loading

alexander-fenster Dec 6, 2019

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 14, 2019

alexander-fenster Dec 6, 2019

schmidt-sebastian Dec 6, 2019

alexander-fenster Dec 6, 2019

schmidt-sebastian left a comment

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 6, 2019

schmidt-sebastian Dec 6, 2019 •

edited

Loading

alexander-fenster left a comment

codecov bot commented Dec 6, 2019 •

edited

Loading

schmidt-sebastian commented Dec 14, 2019

alexander-fenster commented Dec 14, 2019

schmidt-sebastian commented Dec 15, 2019

schmidt-sebastian commented Dec 15, 2019

alexander-fenster commented Dec 15, 2019 •

edited

Loading

alexander-fenster commented Dec 15, 2019

willbattel commented Jun 4, 2020 •

edited

Loading

schmidt-sebastian commented Jun 5, 2020


		'use strict';

feat!: convert Gapic client to TypeScript #805

feat!: convert Gapic client to TypeScript #805

Conversation

alexander-fenster commented Dec 6, 2019 • edited Loading

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 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

schmidt-sebastian Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

alexander-fenster left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2019 • edited Loading

Codecov Report

schmidt-sebastian commented Dec 14, 2019

alexander-fenster commented Dec 14, 2019

schmidt-sebastian commented Dec 15, 2019

schmidt-sebastian commented Dec 15, 2019

alexander-fenster commented Dec 15, 2019 • edited Loading

alexander-fenster commented Dec 15, 2019

willbattel commented Jun 4, 2020 • edited Loading

schmidt-sebastian commented Jun 5, 2020

alexander-fenster commented Dec 6, 2019 •

edited

Loading

schmidt-sebastian Dec 6, 2019 •

edited

Loading

codecov bot commented Dec 6, 2019 •

edited

Loading

alexander-fenster commented Dec 15, 2019 •

edited

Loading

willbattel commented Jun 4, 2020 •

edited

Loading