Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

feat: convert to TypeScript #472

Merged
merged 7 commits into from
Nov 22, 2019
Merged

feat: convert to TypeScript #472

merged 7 commits into from
Nov 22, 2019

Conversation

alexander-fenster
Copy link
Contributor

@alexander-fenster alexander-fenster commented Nov 22, 2019

This one is complicated. It has a very weird dark magic with a helper replacing one of the auto-generated method:

// Augment the SpeechClient objects with the helpers.
for (const gapicVersion of Object.keys(gapic)) {
const clientProto = gapic[gapicVersion].SpeechClient.prototype;
Object.assign(clientProto, helpers());
}

I'm replacing it with an equal (but probably not so dark) magic in TypeScript, making kind of a mixin.

// The following code is adapted from http://www.typescriptlang.org/docs/handbook/mixins.html
// tslint:disable-next-line no-any
Object.defineProperty(
v1.SpeechClient.prototype,
'streamingRecognize',
Object.getOwnPropertyDescriptor(
ImprovedStreamingClient.prototype,
'streamingRecognize'
)!
);
Object.defineProperty(
v1p1beta1.SpeechClient.prototype,
'streamingRecognize',
Object.getOwnPropertyDescriptor(
ImprovedStreamingClient.prototype,
'streamingRecognize'
)!
);

import {ImprovedStreamingClient} from '../helpers';
export interface SpeechClient extends ImprovedStreamingClient {}

Other than that, looks good to me, and all the system tests just work.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 22, 2019
@alexander-fenster
Copy link
Contributor Author

(figuring out how to make samples linted correctly)

@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 22, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 22, 2019
@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #472 into master will increase coverage by 21.68%.
The diff coverage is 91.49%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #472       +/-   ##
===========================================
+ Coverage   69.37%   91.06%   +21.68%     
===========================================
  Files          17       11        -6     
  Lines         209     2932     +2723     
  Branches        0       74       +74     
===========================================
+ Hits          145     2670     +2525     
- Misses         64      260      +196     
- Partials        0        2        +2
Impacted Files Coverage Δ
src/v1/index.ts 100% <100%> (ø)
src/index.ts 100% <100%> (ø)
src/helpers.ts 97.67% <100%> (ø)
src/v1p1beta1/index.ts 100% <100%> (ø)
src/v1/speech_client.ts 93.09% <90.03%> (ø)
src/v1p1beta1/speech_client.ts 93.11% <90.07%> (ø)
samples/betaFeatures.js 82.13% <0%> (ø)
samples/recognize.js 93.96% <0%> (ø)
... and 7 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 cace910...25c718b. Read the comment docs.

package.json Outdated
"system-test": "mocha system-test/*.js --timeout 600000",
"test-no-cover": "mocha test/*.js",
"system-test": "mocha build/system-test/*.js --timeout 600000",
"test-no-cover": "mocha build/test/*.js",
"test": "npm run cover",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just replace this with c8 mocha build/test/*.js, it seems like this has weird bespoke coverage logic (perhaps this was the first module we added coverage to).

// tslint:disable-next-line no-any
Object.defineProperty(
v1.SpeechClient.prototype,
'streamingRecognize',
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the main benefits of the ImprovedStreamingClient mix-in? better types in the response?

I wonder if we should consider upstreaming some of this to the generator itself, worth making a tracking ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a speech-specific change. They want streamingRecognize, which is a bi-di streaming method, to accept a separate config object:

  streamingRecognize(
    streamingConfig?:
      | protosTypes.google.cloud.speech.v1.IStreamingRecognitionConfig
      | protosTypes.google.cloud.speech.v1p1beta1.IStreamingRecognitionConfig,
    options?: gax.CallOptions
  )

By our design, bi-directional streaming calls don't accept any request as a parameter (only as a stream). This one is unique.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants