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

Add Proper types #165

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

sidharthv96
Copy link
Contributor

These have been generated from the json schemas and added to index.d.ts manually.

@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #165 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #165   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          146       146           
  Branches        37        37           
=========================================
  Hits           146       146           

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 496657b...b21b748. Read the comment docs.

Comment on lines +31 to +33
response: {
statusCode: number,
body: any
Copy link

Choose a reason for hiding this comment

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

Handl vault response is using a structure that is similar to the one in ApiResponseError, Do we need a type for this as well?

@kr1sp1n
Copy link
Collaborator

kr1sp1n commented Aug 3, 2020

@sidharthv96 Thanks for your PR 🙏 At the moment I do not care so much about types, so I will review your changes a little bit later. Furthermore what about all those new dev dependencies? Could you please explain why you added them? Thanks.

@sidharthv96
Copy link
Contributor Author

Hi @kr1sp1n , I've added only a single dependency json-schema-to-typescript which converts the json schemas that you have to typescript classes. The diff you see in the yarn.lock files are automatic updates by yarn itself, which can be ignored.

Types are extremely important to our use case and if they are added in the main library, it will be a huge help for us.
As all the interfaces are generated automatically from the schema and are backward compatible with the existing Options type, it'll be easy for review.

You can also run node scripts/gen-types.js to verify the correctness of types.

Thanks for making node-vault!

@jamieweavis
Copy link

jamieweavis commented Dec 1, 2020

Is there any update on this @kr1sp1n? We'd also really appreciate these missing types being added and released!

MichaelSp added a commit to vaultaire/vaultaire that referenced this pull request Apr 24, 2022
@aviadhahami
Copy link
Collaborator

Hey @sidharthv96 -> do you mind resolving the conflicts? <3

@aviadhahami
Copy link
Collaborator

@sidharthv96 another ping ❤️

Copy link
Collaborator

@aviadhahami aviadhahami left a comment

Choose a reason for hiding this comment

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

need PR fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants