Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Basic) Typescript support #511

Merged
merged 3 commits into from
Aug 13, 2021
Merged

Conversation

johnrees
Copy link
Contributor

Description

This adds basic typescript types to the builds. Which helps with autocompletion when using the library.

Kapture.2021-08-13.at.10.19.08.mp4

Without rewriting the codebase in TS (perhaps this would be preferable?) or manually creating and tracking types like the @types project does I don't think it'll ever be perfect, but maybe it's a reasonable start?

Notes

  • I noticed that the npm package doesn't include the dist directory? Is this intentional? I changed package main in the PR because that's where the types get saved to, but it'll mean that dist needs to be included.
  • Parameters aren't optional in the node code, so TS complains when you do something like vultr.account.getAccount() (rather than vultr.account.getAccount({})
  • Most stuff gets typed to :any so TS isn't aware for instance that vultr.account.getAccount({}) returns a Promise, and therefore doesn't autosuggest .then() afterwards

Related Issues

Addresses #393

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #511 (d245c93) into master (fe6341d) will not change coverage.
The diff coverage is n/a.

❗ Current head d245c93 differs from pull request most recent head c5a3341. Consider uploading reports for the commit c5a3341 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #511   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files          24       24           
  Lines         303      303           
  Branches       40       40           
=======================================
  Hits          300      300           
  Misses          3        3           

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 fe6341d...c5a3341. Read the comment docs.

johnrees added a commit to theopensystemslab/vultr-deploy that referenced this pull request Aug 13, 2021
@spencerkordecki
Copy link
Collaborator

@johnrees this is awesome, thanks for the PR! I think this is a good starting point for a rewrite in TS since that would probably be preferable given all the parameters and options we have in the library. I have a local branch that started the process, perhaps now is the time to dust it off and start getting that going again. 🤔

@spencerkordecki spencerkordecki merged commit 3507216 into vultr:master Aug 13, 2021
@johnrees johnrees deleted the typescript branch August 14, 2021 12:59
"main": "src/index.js",
"main": "dist/index.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @spencerkordecki :)

This might break the next publish, until the/dist dir is included in the tarball that's pushed to npm.

I'll try and take a look later if you don't get to it, just wanted to warn you before the next npm publish

@johnrees
Copy link
Contributor Author

@johnrees this is awesome, thanks for the PR! I think this is a good starting point for a rewrite in TS since that would probably be preferable given all the parameters and options we have in the library. I have a local branch that started the process, perhaps now is the time to dust it off and start getting that going again. 🤔

That's great to hear. I'll take a look next week.

I started working on an extremely basic/crude TS api yesterday for an github action that I'm building as an internal PR deployment tool (it won't work without env vars yet), but I'd definitely prefer base it on something that's built & maintained by vultr!

@spencerkordecki spencerkordecki added this to the v2.2.0 milestone Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants