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 typing to all requests #23

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

wodka
Copy link
Contributor

@wodka wodka commented Dec 14, 2021

fixes #7

Copy link

@ifeelfishy ifeelfishy left a comment

Choose a reason for hiding this comment

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

I think this does not account for lists and their specific format in chargebee, or am i mistaken?

src/resources/addon.ts Outdated Show resolved Hide resolved
@wodka
Copy link
Contributor Author

wodka commented Dec 14, 2021

I think this does not account for lists and their specific format in chargebee, or am i mistaken?

I think you are right - will add that as well

@ifeelfishy
Copy link

I think this does not account for lists and their specific format in chargebee, or am i mistaken?

I think you are right - will add that as well

You are a hero! Also the properties 'init_dependant' | 'init_dependant_list' | 'dependant_types' | 'sub_types' | 'values' are inherited from Model but do not appear to be part of the responses.

@wodka
Copy link
Contributor Author

wodka commented Dec 16, 2021

@ifeelfishy I've updated the types now - I actually missed how the Result / ListResult is built and updated that logic now to reflect the types.

image

@ifeelfishy
Copy link

@ifeelfishy I've updated the types now - I actually missed how the Result / ListResult is built and updated that logic now to reflect the types.

image

I missed that too. Very nice.

@jaimefps
Copy link

jaimefps commented Dec 16, 2021

Getting this merged would be a huge win for our team/company. We are ChargeBee paying customers and have had to write a lot of manual typescript to handle the issue of response: any. Would really appreciate if this PR moves along.

@ifeelfishy
Copy link

@wodka Also, how does it play with the lazy loading feature of this library? From what i see in the usage

await c.addon.list().request()

will return {list: [{},{},{} ...], next_offset}, with list of empty objects

@wodka
Copy link
Contributor Author

wodka commented Dec 22, 2021

@ifeelfishy I guess you just did a console.log on it right? It does look super strange when doing a console.log on it - but this change doesn't modify that behaviour

import { ListResult } from './lib/list_result'
import { ChargeBee } from './src/chargebee'

(async () => {
  console.log('just to check!!')

  const c = new ChargeBee()

  c.configure()

  const result = await c.addon.list().request()

  console.log(result)
  console.log(result.list[0].addon)
})()

will reduce the following:

image

@L-U-C-K-Y
Copy link

@cb-khushbubibay @cb-rakesh @cb-goutham @cb-yateshmathuria @cb-navaneedhan

We are also paying customers and our main integration issues would be solved with this PR, please help and merge.

@cb-khushbubibay
Copy link
Contributor

@cb-khushbubibay @cb-rakesh @cb-goutham @cb-yateshmathuria @cb-navaneedhan

We are also paying customers and our main integration issues would be solved with this PR, please help and merge.

@L-U-C-K-Y
Thanks for being patient, We are working on it and will release it ASAP.

@wodka
Copy link
Contributor Author

wodka commented Jan 12, 2022

@cb-khushbubibay @cb-rakesh @cb-goutham @cb-yateshmathuria @cb-navaneedhan
We are also paying customers and our main integration issues would be solved with this PR, please help and merge.

@L-U-C-K-Y Thanks for being patient, We are working on it and will release it ASAP.

out of curiosity - what are you working on there?

I see the main reason for pull requests that the community (I am a paying customer as well) prepares the changes in the way you also like them so it can easily be added to the project.

@cb-khushbubibay
Copy link
Contributor

@cb-khushbubibay @cb-rakesh @cb-goutham @cb-yateshmathuria @cb-navaneedhan
We are also paying customers and our main integration issues would be solved with this PR, please help and merge.

@L-U-C-K-Y Thanks for being patient, We are working on it and will release it ASAP.

out of curiosity - what are you working on there?

I see the main reason for pull requests that the community (I am a paying customer as well) prepares the changes in the way you also like them so it can easily be added to the project.

@wodka
Thank you for your time to create PR. Really appreciate it.
Team is reviewing changes and we will address this PR/Issue in the upcoming releases.

@cb-yateshmathuria cb-yateshmathuria merged commit 9c37100 into chargebee:master Jan 21, 2022
@wodka
Copy link
Contributor Author

wodka commented Jan 21, 2022

thank you @cb-yateshmathuria and @cb-khushbubibay for reviewing and merging :)

@ifeelfishy
Copy link

@ifeelfishy I guess you just did a console.log on it right? It does look super strange when doing a console.log on it - but this change doesn't modify that behaviour

import { ListResult } from './lib/list_result'
import { ChargeBee } from './src/chargebee'

(async () => {
  console.log('just to check!!')

  const c = new ChargeBee()

  c.configure()

  const result = await c.addon.list().request()

  console.log(result)
  console.log(result.list[0].addon)
})()

will reduce the following:

image

yes, thats what i did... super wierd

@schourode
Copy link

I am working on a project that uses chargebee-typscript v2.33, and don't see any (usable) types on API responses from Chargebee. Take this example:

const { subscription } = await client.subscription.retrieve(userId).request();

VSCode believes subscription to be of type Promise<Promise<T>>. The T in there seems to be somehow recursive, so that putting await in front of the value will yield Promise<T>, which can again be await'ed to Promise<T> and so forth. Attempting to access any property yields in a value of type any.

If I understand this PR correctly, it was meant to add proper types to responses. Were these types later lost in a regression? Or is this happening only on my machine due to some unfortunate combination of packages installed or similar?

@cb-sriramthiagarajan
Copy link
Collaborator

Hi @schourode, thanks for raising this. Yes, this seems to be broken. We're looking into this. Will keep this thread posted.

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.

[Types] Add return types to api requests
8 participants