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

refactor: migrate to TypeScript #534

Closed
wants to merge 13 commits into from

Conversation

roikoren755
Copy link

@roikoren755 roikoren755 commented Jan 4, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #495 🦕

@roikoren755 roikoren755 requested a review from a team as a code owner January 4, 2021 20:07
@google-cla
Copy link

google-cla bot commented Jan 4, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@product-auto-label product-auto-label bot added the api: compute Issues related to the googleapis/nodejs-compute API. label Jan 4, 2021
@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Jan 4, 2021
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #534 (f216945) into master (d1d9f4c) will decrease coverage by 1.60%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
- Coverage   99.61%   98.00%   -1.61%     
==========================================
  Files          22       24       +2     
  Lines       11944    12793     +849     
  Branches        0      446     +446     
==========================================
+ Hits        11898    12538     +640     
- Misses         46      230     +184     
- Partials        0       25      +25     
Impacted Files Coverage Δ
src/index.ts 0.00% <0.00%> (ø)
src/interfaces.ts 0.00% <0.00%> (ø)
src/operation.ts 93.11% <75.00%> (ø)
src/rule.ts 98.82% <93.33%> (ø)
src/vm.ts 98.82% <97.42%> (ø)
src/instance-group-manager.ts 99.76% <98.30%> (ø)
src/compute.ts 99.32% <98.43%> (ø)
src/autoscaler.ts 99.70% <98.43%> (ø)
src/firewall.ts 99.69% <98.43%> (ø)
src/region.ts 99.59% <98.82%> (ø)
... and 15 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 d1d9f4c...f216945. Read the comment docs.

@roikoren755
Copy link
Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jan 4, 2021
@roikoren755
Copy link
Author

There are still a few issues I need to resolve, namely a few // @ts-ignore comments I inserted to remind me of type mismatches. Hoping to get those out of the way soon

@roikoren755 roikoren755 changed the title Migrate to TypeScript refactor: migrate to TypeScript Jan 4, 2021
@roikoren755
Copy link
Author

roikoren755 commented Jan 5, 2021

Regarding the failing checks -

  • There's still one error in ESLint, which I'm not sure how to solve. In the Compute class, in the constructor, the library's package.json is required and saved to a field. Since the library in now transpiled, the output is placed inside a build dir. The package.json file isn't moved, and so cannot be found when requiring '../package.json', instead needing to require it from '../../package.json', as was done in @google-cloud/storage. Not sure how to resolve this issue.
  • Not sure how to fix the header-check check, tried changing the license header in the files it errors on, could not get it to work.
  • And finally, code coverage - I created an interfaces.ts file that isn't tested at all, moved the old index.js to compute.ts, and created a new index.ts file that exports everything, without holding any code. This file, likewise, is not tested. This lowers the coverage, unfortunately. Not sure about other files, where interfaces and function overload signatures were added.
  • No idea about the docs failing, either...

@bcoe
Copy link
Contributor

bcoe commented Jan 6, 2021

@roikoren755 appreciate the PR 👍

It might be a little while before we can give a thorough review. There has been some work internally to potentially auto-generate compute, in the not-too-distant future as well (which would generate it with types).

I'd like to figure out where this work stands.

@oveddan
Copy link

oveddan commented Jan 7, 2021

Thanks for working on this PR! I hope typescript is supported soon in this sdk, it would make things much easier.

@bcoe
Copy link
Contributor

bcoe commented Jan 12, 2021

@roikoren755 before we dive into the review of this PR, I was wondering if you might be able to take a look at the TypeScript support we're planning for the next major version:

#537

Installable here:

https://www.npmjs.com/package/@google-cloud/compute/v/3.0.0-alpha.1


I discussed this PR with my peers this week, we're open to landing this functionality. But, we think we're potentially only a few weeks out on landing #537, which moves us from a handwritten library with out TypeScript types, to an automatically generated library with TypeScript types.

@roikoren755
Copy link
Author

@roikoren755 before we dive into the review of this PR, I was wondering if you might be able to take a look at the TypeScript support we're planning for the next major version:

#537

Installable here:

https://www.npmjs.com/package/@google-cloud/compute/v/3.0.0-alpha.1


I discussed this PR with my peers this week, we're open to landing this functionality. But, we think we're potentially only a few weeks out on landing #537, which moves us from a handwritten library with out TypeScript types, to an automatically generated library with TypeScript types.

I can take a look at it over the weekend. What exactly would you like me to check? The use case we have in my company is a fairly simple one

@bcoe
Copy link
Contributor

bcoe commented Jan 14, 2021

What exactly would you like me to check? The use case we have in my company is a fairly simple one

@roikoren755 perhaps you could see if the new generated library meets your company's needs, in which case perhaps we could support you in using it (or fix issues based on your feedback).

@jasonswearingen
Copy link

What exactly would you like me to check? The use case we have in my company is a fairly simple one

@roikoren755 perhaps you could see if the new generated library meets your company's needs, in which case perhaps we could support you in using it (or fix issues based on your feedback).

I tried using 3.0.0-alpha.1.... it looks like it will be fine if you add some better samples. The samples included in the readme.md are too trivial to explain the patterns the new api designs expect, and there seems to be no other documentation included. (All other docs looks to refer to the previous api)

Also, How can I access the compute Beta API ?

@SurferJeffAtGoogle
Copy link
Contributor

This PR has been inactive for 3+ months.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: compute Issues related to the googleapis/nodejs-compute API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing typescript declaration file
5 participants