Skip to content
This repository has been archived by the owner on Dec 19, 2023. It is now read-only.

feat!: move library to typescript code generation #285

Merged
merged 26 commits into from
Nov 14, 2019
Merged

Conversation

xiaozhenliu-gg5
Copy link
Contributor

The basic API interface

import {TextToSpeechClient} from '@google-cloud/text-to-speech';
// Import other required libraries
import  * as fs from 'fs';
import * as util from 'util';
async function main() {
  // Creates a client
  const client = new TextToSpeechClient();

  // The text to synthesize
  const text = 'Hello, world!';

  // Construct the request
  const request = {
    input: {text: text},
    // Select the language and SSML Voice Gender (optional)
    voice: {languageCode: 'en-US', ssmlGender: 'NEUTRAL'},
    // Select the type of audio encoding
    audioConfig: {audioEncoding: 'MP3'},
  };

  // Performs the Text-to-Speech request
  const [response] = await client.synthesizeSpeech(request);
  // Write the binary audio content to a local file
  const writeFile = util.promisify(fs.writeFile);
  await writeFile('output.mp3', response.audioContent, 'binary');
  console.log('Audio content written to file: output.mp3');
}

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2019
@xiaozhenliu-gg5 xiaozhenliu-gg5 requested a review from bcoe November 8, 2019 01:16
synth.py Outdated Show resolved Hide resolved
@alexander-fenster
Copy link
Contributor

@bcoe @JustinBeckwith 👏
This is the first actual library conversion. Since this one had @types/ published it will go as major.

package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/v1/text_to_speech_client.ts Outdated Show resolved Hide resolved
src/v1/text_to_speech_client.ts Outdated Show resolved Hide resolved
synth.py Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@alexander-fenster
Copy link
Contributor

@xiaozhenliu-gg5 what's up with src/browser.js? Don't we generate a .ts? (if we do, we should also add "browser": "build/src/browser.js" to package.json and hope that it just works :) )

@bcoe bcoe self-assigned this Nov 11, 2019
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I will give thorough review as soon as I get through some routine weekly tasks.

One note, I think we should hold off on moving over any existing libraries, until we have jsdoc added back; It's fine for a few edge-cases, like adding the new v3 API to translate, or generating a brand new library; but for existing libraries with good reference documentation, keeping the same level of quality is important (I know this work is coming along any ways 😄 )

@alexander-fenster
Copy link
Contributor

@bcoe jsdoc is there! I will take a deeper look later today at what is actually being generated by jsdoc, and likely make fixes, but the general structure of jsdoc comments is already here.

@alexander-fenster
Copy link
Contributor

This will also wait for googleapis/gapic-generator-typescript#125 (webpack.js is not taken care of). This will be a quick fix for the generator.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

@alexander-fenster with regards to jsdoc, awesome 😄

we should run npm run docs locally, and see how we feel about the docs (as part of our work).

@JustinBeckwith
Copy link
Contributor

Looks like we also need to address the missing license headers:

protos/protos.d.ts is missing a valid license header.
system-test/fixtures/sample/src/index.ts is missing a valid license header.
system-test/install.ts is missing a valid license header.

@alexander-fenster
Copy link
Contributor

@JustinBeckwith License headers in the generated files are addressed in googleapis/gapic-generator-typescript#124.

As for protos.d.ts, I'm not sure it needs to have a license header at all, but if it must, I can add it in gax (compileProtos.ts script generates it). Do you think it's needed, or it's better to ignore .d.ts in the bot?

@JustinBeckwith
Copy link
Contributor

That's more of a question for OSPO than me :)

@alexander-fenster
Copy link
Contributor

OK! License for protos.d.ts is taken care of by googleapis/gax-nodejs#649, then gax v1.9.1 will go out and I'll update this PR.

@xiaozhenliu-gg5
Copy link
Contributor Author

For running locally npm run docs and comparison between googlapis.dev/nodejs/text-to-speech, generated comments are parsed by jsdoc correctly. And small inconsistency would be taken care by googleapis/gapic-generator-typescript#127 @bcoe @alexander-fenster

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

this is looking really promising to me, it's great that all the sample tests pass without any changes (speaks to the API surface staying consistent).

package.json Outdated Show resolved Hide resolved
system-test/install.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #285 into master will increase coverage by 12.95%.
The diff coverage is 88.47%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #285       +/-   ##
===========================================
+ Coverage   75.88%   88.83%   +12.95%     
===========================================
  Files           8        5        -3     
  Lines         141      896      +755     
  Branches        0       53       +53     
===========================================
+ Hits          107      796      +689     
- Misses         34       98       +64     
- Partials        0        2        +2
Impacted Files Coverage Δ
src/v1/index.ts 100% <100%> (ø)
src/index.ts 100% <100%> (ø)
src/v1beta1/index.ts 100% <100%> (ø)
src/v1/text_to_speech_client.ts 87.86% <87.86%> (ø)
src/v1beta1/text_to_speech_client.ts 88.09% <88.09%> (ø)
.../doc/google/cloud/texttospeech/v1/doc_cloud_tts.js
...google/cloud/texttospeech/v1beta1/doc_cloud_tts.js
... and 2 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 c76c5ca...c7f9cda. Read the comment docs.

@alexander-fenster alexander-fenster self-requested a review November 12, 2019 20:50
@alexander-fenster alexander-fenster changed the title feat: move library to typescript code generation feat!: move library to typescript code generation Nov 13, 2019
@alexander-fenster
Copy link
Contributor

@xiaozhenliu-gg5 @bcoe @JustinBeckwith I hereby declare this library ready! :)

Three things we'll keep working on are:

  • package.json improvements - it's mostly done, new packages will have system-test script, linkinator stuff already there, but we are adding more stuff there.
  • generating main src/index.ts for multiple versions - we are thinking what's the best way of doing it.
  • one s.replace in synth.py will be taken care of by accept an option for package name gapic-generator-typescript#134

With these three things in mind, I think we can go ahead and release this as a semver major (as discussed, because of DefinitelyTyped types). What do you folks think?

.jsdoc.js Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
"\(https:[\s\*]+(.*)\)",
r"(https:\1)")
# Fix system tests
# TODO: must be a feature of pack-n-play
Copy link
Contributor

Choose a reason for hiding this comment

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

What's missing here? Do we need a tracking bug over in pack-n-play?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a tracking issue here googleapis/gapic-generator-typescript#134 - undecided if it's gonna be in pack-n-play or an option for the generator. The problem is that the generator does not know the package name 😿

@alexander-fenster
Copy link
Contributor

It's ready for another look @JustinBeckwith :)

@alexander-fenster alexander-fenster merged commit d3d6208 into master Nov 14, 2019
@alexander-fenster alexander-fenster deleted the ts_lib branch November 14, 2019 23:32
@release-please release-please bot mentioned this pull request Nov 14, 2019
@JustinBeckwith
Copy link
Contributor

Congrats folks!

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.

5 participants