chore!: update settings for Node 10 #1019

schmidt-sebastian · 2020-04-14T16:26:51Z

This PR updates the synth.py script to work with the new format/tooling brought by the Node 10 migration.

Manual changes are in synth.py and dev/protos/update.sh to enable namespacing and add toObject and fromObject (which is needed for the generated unit tests and unfortunately adds a lot of code - maybe we can find a way to remove this code in a follow up).

schmidt-sebastian · 2020-04-14T16:48:06Z

Hm, this is completely busted. I will fix this later.

JustinBeckwith · 2020-04-14T21:44:15Z

.eslintignore

 build/
 docs/
 protos/
+system-test/


The tooling is gonna undo that. Any reason to explicitly ignore it?

This should be from the tooling. I didn't make this change manually at least :)

Hmmm, we don't do it in other libs. Example: https://github.com/googleapis/nodejs-text-to-speech/blob/master/.eslintignore

I deleted this file and re-ran synthtool and it was recreated just like this.

JustinBeckwith · 2020-04-14T22:27:31Z

synth.py

@@ -98,6 +138,13 @@
  1
 )

+os.rename("dev/.gitignore", ".gitignore")


Not sure what all goes here, but this may be missing .prettierignore

I take that back: That file doesn't exist:

FileNotFoundError: [Errno 2] No such file or directory: 'dev/.prettierignore' -> '.prettierignore'

I guess we should add this to our templates?

codecov · 2020-04-15T20:02:25Z

Codecov Report

Merging #1019 into node10 will increase coverage by 98.54%.
The diff coverage is 99.01%.

@@             Coverage Diff             @@
##           node10    #1019       +/-   ##
===========================================
+ Coverage    0.00%   98.54%   +98.54%     
===========================================
  Files          25       25               
  Lines       16044    16614      +570     
  Branches        0     1258     +1258     
===========================================
+ Hits            0    16372    +16372     
+ Misses      16044      239    -15805     
- Partials        0        3        +3

Impacted Files	Coverage Δ
dev/src/v1/firestore_client.ts	`97.64% <98.96%> (+97.64%)`	⬆️
dev/src/v1beta1/firestore_client.ts	`97.38% <99.01%> (+97.38%)`	⬆️
dev/src/v1/firestore_admin_client.ts	`97.71% <99.07%> (+97.71%)`	⬆️
dev/src/transaction.ts	`96.49% <0.00%> (+96.49%)`	⬆️
dev/src/pool.ts	`97.75% <0.00%> (+97.75%)`	⬆️
dev/src/validate.ts	`98.05% <0.00%> (+98.05%)`	⬆️
dev/src/order.ts	`98.09% <0.00%> (+98.09%)`	⬆️
... and 18 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 5fbc6e6...cd88990. Read the comment docs.

schmidt-sebastian · 2020-04-15T20:28:26Z

@alexander-fenster Can you sanity check the manual changes?

alexander-fenster · 2020-04-15T22:15:56Z

.jsdoc.js

    includeDate: false,
    sourceFiles: false,
-    systemName: '@google-cloud/firestore',
+    systemName: 'firestore',


For this, you need to add "package-name": "@google-cloud/firestore" to the generator arguments in synth.py.

Done (and this fixed the issue here)

alexander-fenster · 2020-04-15T22:17:09Z

dev/protos/update.sh

  "${PROTOS_DIR}/google/firestore/admin/v1/*.proto" \
  "${PROTOS_DIR}/google/protobuf/*.proto" "${PROTOS_DIR}/google/type/*.proto" \
  "${PROTOS_DIR}/google/rpc/*.proto" "${PROTOS_DIR}/google/api/*.proto" \
  "${PROTOS_DIR}/google/longrunning/*.proto"
 "${PBTS}" -o firestore_admin_v1_proto_api.d.ts firestore_admin_v1_proto_api.js

 "${PBJS}" "${PBJS_ARGS[@]}" -o firestore_v1beta1_proto_api.js \
+  -r firestore_v1beta1_v1 \


Does not really affect anything but the name is weird. Maybe just firestore_v1beta1?

alexander-fenster · 2020-04-15T22:18:50Z

dev/src/v1/firestore_admin_client.ts

+   * @returns {Object}
+   *   An iterable Object that conforms to @link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols.
+   */
+  listIndexesAsync(


FYI @schmidt-sebastian: we now support this :) for await (const index of client.listIndexesAsync(request)) - feel free to use this in your manual layer! (the same for any list* paginated method)

Oh, awesome. Thanks for pointing this out. We should probably use expose this in our APIs that currently hide all paging, since we didn't want to use Streams or Callbacks as the primary way of interacting with our APIs.

alexander-fenster · 2020-04-15T22:19:38Z

package.json

@@ -5,7 +5,7 @@
  "license": "Apache-2.0",
  "author": "Google Inc.",
  "engines": {
-    "node": "^8.13.0 || >=10.10.0"
+    "node": ">=10.10.0"


This is so breaking. The title must be chore! then :)

Changed. Note that the base branch has a ! already.

alexander-fenster · 2020-04-15T22:23:44Z

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.ts",
-    "return this\._innerApiCalls\.listen\(options\);",
-    "return this._innerApiCalls.listen({}, options);",
+    "return this\.innerApiCalls\.listen\(options\);",


Every time I see this replacement I wonder what's wrong with the code here, why we need this replacement?

I do think the generated code is busted here. We use options to set an additional header, but the way this gets passed down here is as the request (if I remember this correctly). If we don't modify the code, the header is dropped. This only affects bidirectional streaming APIs with additional metadata, so probably just us. I do think we should fix this upstream at some point.

OK, this sounds like a bug - filed googleapis/gapic-generator-typescript#425 and one of us will investigate.

alexander-fenster · 2020-04-15T22:30:06Z

Manual changes are somewhat scary :) A lot of changes are caused by the fact that the gapic code lives in dev/src/. I'm just thinking, is the directory structure fixed in stone here? Other manual packages (spanner, pubsub) have their sources in src/, and their synth.py files are tiny and simple. Maybe not right now, but let's at least have a plan there?

(an alternative is to teach the generator how to place files into a separate location, but I can't estimate how hard it would be)

schmidt-sebastian

Re: package structure. The main reason for our dev/ and build/ package structure is that it allows us to import the package.json in a consistent way. If I parsed the discussions on Yoshi chat correctly this also now works around a breakage in npm pack, so I am not super keen on changing this :)

schmidt-sebastian · 2020-04-15T22:36:07Z

.jsdoc.js

    includeDate: false,
    sourceFiles: false,
-    systemName: '@google-cloud/firestore',
+    systemName: 'firestore',


Done (and this fixed the issue here)

schmidt-sebastian · 2020-04-15T22:36:30Z

package.json

@@ -5,7 +5,7 @@
  "license": "Apache-2.0",
  "author": "Google Inc.",
  "engines": {
-    "node": "^8.13.0 || >=10.10.0"
+    "node": ">=10.10.0"


Changed. Note that the base branch has a ! already.

schmidt-sebastian · 2020-04-15T22:37:15Z

.eslintignore

 build/
 docs/
 protos/
+system-test/


I deleted this file and re-ran synthtool and it was recreated just like this.

schmidt-sebastian · 2020-04-15T22:40:09Z

dev/src/v1/firestore_admin_client.ts

+   * @returns {Object}
+   *   An iterable Object that conforms to @link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols.
+   */
+  listIndexesAsync(


Oh, awesome. Thanks for pointing this out. We should probably use expose this in our APIs that currently hide all paging, since we didn't want to use Streams or Callbacks as the primary way of interacting with our APIs.

schmidt-sebastian · 2020-04-15T22:40:23Z

dev/protos/update.sh

  "${PROTOS_DIR}/google/firestore/admin/v1/*.proto" \
  "${PROTOS_DIR}/google/protobuf/*.proto" "${PROTOS_DIR}/google/type/*.proto" \
  "${PROTOS_DIR}/google/rpc/*.proto" "${PROTOS_DIR}/google/api/*.proto" \
  "${PROTOS_DIR}/google/longrunning/*.proto"
 "${PBTS}" -o firestore_admin_v1_proto_api.d.ts firestore_admin_v1_proto_api.js

 "${PBJS}" "${PBJS_ARGS[@]}" -o firestore_v1beta1_proto_api.js \
+  -r firestore_v1beta1_v1 \


schmidt-sebastian · 2020-04-15T22:42:28Z

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.ts",
-    "return this\._innerApiCalls\.listen\(options\);",
-    "return this._innerApiCalls.listen({}, options);",
+    "return this\.innerApiCalls\.listen\(options\);",


I do think the generated code is busted here. We use options to set an additional header, but the way this gets passed down here is as the request (if I remember this correctly). If we don't modify the code, the header is dropped. This only affects bidirectional streaming APIs with additional metadata, so probably just us. I do think we should fix this upstream at some point.

* fix!: mark v1beta1 client as deprecated (#937) * feat!: use QueryDocumentSnapshot in FirestoreDataConverter (#965) * deps: update to gts 2.x (#1013) * chore!: update settings for Node 10 (#1019) * deps: drop through2 (#1014) * feat: support BigInt (#1016) * fix: make update.sh work on Linux (#1043) * fix: only use BigInt in BigInt system test (#1044) * fix: make pbjs compile admin proto again (#1045) * Add BulkWriter (#1055) * docs: Add documentation for FirestoreDataConverter (#1059) * chore: enforce return types (#1065) * fix: add generic to Firestore.getAll() (#1066) * chore: remove internal WriteOp (#1067) * chore: add linter checks for it|describe.only (#1068) * fix: handle terminate in BulkWriter (#1070) * chore: run template copying last in synthtool (#1071) * feat: Firestore Bundles implementation (#1078) * feat: add support for set() with SetOptions when using FirestoreDataConverter (#1087) * feat: Add totalDocuments and totalBytes to bundle metadata. (#1085) * feat: Add totalDocuments and totalBytes to bundle metadata. * fix: Better comment * fix: Better testing. * fix: Improve metadata testing. * fix: incomplete expect in rate-limiter test (#1092) * Remove BatchWrite proto, fix conformance tests * chore: use public API types internally (#1100) * feat: add Partition and BatchWrite protos (#1110) * fix: remove GCF transaction fallback (#1112) * fix: add BulkWriter integration tests, java backport changes, delete fix (#1117) * chore: merge master (#1218) * chore: add eslint check for console.log statements (#1229) * fix: another attempt at fixing the flaky BulkWriter test (#1228) * Fix comment * Renames * Test fix * Fix unit tests Co-authored-by: Brian Chen <[email protected]> Co-authored-by: wu-hui <[email protected]>

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2020

chore: update settings for Node 10

ead628f

schmidt-sebastian force-pushed the mrschmidt/node10synthtool branch from b8db912 to ead628f Compare April 14, 2020 16:27

schmidt-sebastian requested a review from JustinBeckwith April 14, 2020 16:28

schmidt-sebastian assigned JustinBeckwith Apr 14, 2020

schmidt-sebastian marked this pull request as draft April 14, 2020 16:47

JustinBeckwith approved these changes Apr 14, 2020

View reviewed changes

schmidt-sebastian added 2 commits April 14, 2020 14:36

Update synth.py

0793d04

WIP

c4c1fc8

JustinBeckwith approved these changes Apr 14, 2020

View reviewed changes

fix tests

c380a8a

Fix docs

eb77d86

schmidt-sebastian marked this pull request as ready for review April 15, 2020 20:28

schmidt-sebastian requested a review from alexander-fenster April 15, 2020 20:28

schmidt-sebastian assigned alexander-fenster Apr 15, 2020

schmidt-sebastian unassigned JustinBeckwith Apr 15, 2020

alexander-fenster reviewed Apr 15, 2020

View reviewed changes

schmidt-sebastian changed the title ~~chore: update settings for Node 10~~ chore!: update settings for Node 10 Apr 15, 2020

alexander-fenster reviewed Apr 15, 2020

View reviewed changes

Review

cd88990

schmidt-sebastian commented Apr 15, 2020

View reviewed changes

alexander-fenster mentioned this pull request Apr 15, 2020

bi-directional streaming calls with extra options googleapis/gapic-generator-typescript#425

Closed

alexander-fenster approved these changes Apr 15, 2020

View reviewed changes

schmidt-sebastian merged commit 7d6888d into node10 Apr 15, 2020

schmidt-sebastian deleted the mrschmidt/node10synthtool branch April 15, 2020 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: update settings for Node 10 #1019

chore!: update settings for Node 10 #1019

schmidt-sebastian commented Apr 14, 2020 •

edited

Loading

schmidt-sebastian commented Apr 14, 2020

JustinBeckwith Apr 14, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

JustinBeckwith Apr 14, 2020

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

codecov bot commented Apr 15, 2020 •

edited

Loading

schmidt-sebastian commented Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

schmidt-sebastian Apr 15, 2020

alexander-fenster Apr 15, 2020

alexander-fenster commented Apr 15, 2020

schmidt-sebastian left a comment

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

schmidt-sebastian Apr 15, 2020

chore!: update settings for Node 10 #1019

chore!: update settings for Node 10 #1019

Conversation

schmidt-sebastian commented Apr 14, 2020 • edited Loading

schmidt-sebastian commented Apr 14, 2020

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 Apr 15, 2020 • edited Loading

Codecov Report

schmidt-sebastian commented Apr 15, 2020

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

alexander-fenster commented Apr 15, 2020

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

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 15, 2020 •

edited

Loading