[Done] API response types + Typescript

[jameslinimk]

Hey, I've been using this package for a bit and saw there were no types, so I decided to make them. I was able to generate a Typescript client from the Swagger Docs, but some responses are incomplete, so I'm just going through them one by one.

Methods done

• All done

[raimannma]

Hey @jameslinimk,

Thanks for the Typescript rewrite many of us are waiting for it :D

I went through the whole code and saw some bugs and Indentation errors.

Indentation

First of all, this package does not use tabs to indent the code. It uses 4 spaces, so you should too.
@Henrik-3 Should add a editorconfig file to specify this.

And also your indentations are sometimes wrong and the code is horrible to read at that parts.
Maybe adding a pre-commit hook?

Typing

Your types are looking good so far (saw one bug, see comment).
Just for you too know we have all those types already in Python code here: https://github.com/raimannma/ValorantAPI/tree/master/valo_api/responses

[raimannma]

Maybe doing this a bit more dynamically, cause you have to add an episode id every season

[jameslinimk]

It isn't. It just extends the existing KillEvent (that doesnt have the attribute round)

[raimannma]

It isn't. It just extends the existing KillEvent (that doesnt have the attribute round)

Oh yeah I didn't saw the extend.

[raimannma]

This should also be dynamic via a get request to valorant-api.com

[raimannma]

Why 3 times?

[jameslinimk]

Honestly, I don't know. I just took this from the typescript website, I'll fix it

[raimannma]

I think just the first line is enough.

[raimannma]

Should be removed or replaced by real tests

[jameslinimk]

I was just using it to make sure the function works, I'll write real tests soon

[raimannma]

There are multiple Indentation errors in this file.

[jameslinimk]

See my comment

[jameslinimk]

See my comment

[raimannma]

Yeah I think this is ok, it wouldn't change.

[raimannma]

Why are you importing javascript files here?

Also this files doesn't even exist.

[jameslinimk]

See my comment

[raimannma]

Why are you importing javascript files here?

Also this files doesn't even exist.

[raimannma]

Why are you importing javascript files here?

Also this files doesn't even exist.

[raimannma]

Why are you importing javascript files here?

Also this files doesn't even exist.

[raimannma]

Why are you importing javascript files here?

Also this files doesn't even exist.

[raimannma]

Ignoring all js files?? When there are js files in this project?

[jameslinimk]

See my comment

[jameslinimk]

1. Sorry about the inconsistent spacing. Ill add eslint
2. I'll take a look at the python typings and use something like py-ts-interfaces to help me
3. I have to import from JS when working with relative imports see here
4. The reason I ignore JS files, is to compile them to .js and .d.ts files, and those are going to be the files uploaded to npm, but I was going to do that once I finished
5. The "85ad13f7-3d1b-5128-9eb2-7cd8ee0b5741": number is if you look at the API result you can see that 85ad13f7-3d1b-5128-9eb2-7cd8ee0b5741 is in every Cost object. I believe this is because this is the ID of Valorant Points.
6. I don't think there is a better way to improve the Rank typing, other than manually updating it every month. I could create a github workflow that automatically checks for a new act/episode and edits the type.
7. For the map images and rank images, I'll change that to calls to valorant-api

[jameslinimk]

Also, do you have any answers for the 2 issues in the first message?

[raimannma]

1. Sorry about the inconsistent spacing. Ill add eslint
2. I'll take a look at the python typings and use something like py-ts-interfaces to help me
3. I have to import from JS when working with relative imports see here
4. The reason I ignore JS files, is to compile them to .js and .d.ts files, and those are going to be the files uploaded to npm, but I was going to do that once I finished
5. The "85ad13f7-3d1b-5128-9eb2-7cd8ee0b5741": number is if you look at the API result you can see that 85ad13f7-3d1b-5128-9eb2-7cd8ee0b5741 is in every Cost object. I believe this is because this is the ID of Valorant Points.
6. I don't think there is a better way to improve the Rank typing, other than manually updating it every month. I could create a github workflow that automatically checks for a new act/episode and edits the type.
7. For the map images and rank images, I'll change that to calls to valorant-api

1. That would be good
2. You would probably use absolute imports, I don't think it's a good convention to use that .js appendix
3. Simply build them into a build folder and ignore that. But yeah after everything is rewritten in typescript this ignore works too.
4. I am not sure but maybe this value can change, I think @Henrik-3 knows better.
5. You could type it like e[number]a[number] ? Or isn't that explicit enough ?

[jameslinimk]

2. Absolute imports wouldn't change anything, though because only the .d.ts files have imports, Ill remove the .js extension. Also, importing the js files is very common and most projects that don't compile to js and .d.ts do it.
3. From my tests, no
4. I guess I could, tho, I would use e${number}a${"1" | "2"| "3"}

I plan to finish all the methods by today

[jameslinimk]

@raimannma I just finished implementing all the methods and making changes, may you review them?

Also, I have 2 questions,

1. What does the endpoint v1/store-offers return? I assumed by looking at the response that it returned all in-app-purchases, but I might be wrong. The swagger docs say nothing about it.
2. v2/store-featured seems to be bugged (I get an error "TypeError: dataset.levels.find is not a function"), but v1/store-featured works fine. Is this intentional?

[raimannma]

The dist folder should only contain automatically built files and should not be tracked via git in my understanding.

[jameslinimk]

The dist folder should only contain automatically built files and should not be tracked via git in my understanding.

Oh yeah, I forgot about that. I'm currently working on better comments using tsdoc instead of jsdoc. Perhaps documentation can be made typedoc if you would like. After that, I believe it should be ready

[jameslinimk]

@raimannma Done. I changed the README a bit, if you wouldn't mind taking a look. I'm going to go to bed now.

[raimannma]

If ou use this you should delete the other git push action

[jameslinimk]

Done

[Henrik-3]

Holy Shit what happened here....
I am back at home now but it's already like 2am here, so i am gonna review the stuff tommorow or on Sunday, but all in all it looks pretty promising.
One thing i will change is the versioning since it should be in line with current API Version which 3.0.0 isn't, but i am gonna request the changes tommorow if needed.

I have to be honest i never used TS for big projects like because of the lack of time to learn the TS stuff so for the next API update i am gonna be fcked :D

@jameslinimk if u wish i will give u the Package Maintainer Role on the server if you want to maintain the package in the future (for sure i will create pull requests for further packages if you don't want to maintain it but then i can't guarantee the correctness in regard of TS stuff until i learned it which will need some time)

[raimannma]

Holy Shit what happened here.... I am back at home now but it's already like 2am here, so i am gonna review the stuff tommorow or on Sunday, but all in all it looks pretty promising. One thing i will change is the versioning since it should be in line with current API Version which 3.0.0 isn't, but i am gonna request the changes tommorow if needed.

I have to be honest i never used TS for big projects like because of the lack of time to learn the TS stuff so for the next API update i am gonna be fcked :D

@jameslinimk if u wish i will give u the Package Maintainer Role on the server if you want to maintain the package in the future (for sure i will create pull requests for further packages if you don't want to maintain it but then i can't guarantee the correctness in regard of TS stuff until i learned it which will need some time)

I can also help with the TS stuff.

[Henrik-3]

Feel free to do so :D

[raimannma]

Just also for completeness here is the link to the auto generated docs from @jameslinimk's fork: https://jameslinimk.github.io/unofficial-valorant-api/

[raimannma]

I also have a comment about endpoint versioning.

In the current code you only are able to fetch the latest endpoint version so for leaderboard v2, for mmr v2 and so.
It would be also good to support all the older versions as some are using them. I saw that you did this for the leaderboard, by creating a leaderboard v1 method.

But that should be possible for all endpoints.

Also I would like to add the version to the method name, cause that decreases compatibility issues.

If @Henrik-3 adds a new endpoint version to the api and changes the endpoint also in this package, then the users automatically fetch the new endpoint.
But if the methods have their version in name they fetch the same endpoint even after updating their dependencies.

I hope this is understandable.

See also my method naming in the Python Wrapper:

• https://raimannma.github.io/ValorantAPI/_source_files/valo_api.endpoints.html#module-valo_api.endpoints.leaderboard
• https://raimannma.github.io/ValorantAPI/_source_files/valo_api.endpoints.html#module-valo_api.endpoints.mmr_details

[Henrik-3]

As a reminder: The current version should be 100% compatible with the last update, so we kinda need a version field in the parameter.

We can change this in the future with a new major release or add them as an optional thing

[raimannma]

Also what needs to be done is automatic deployment to npm or at least a script to do so, but maybe doing that in a seperate PR.

[raimannma]

As a reminder: The current version should be 100% compatible with the last update, so we kinda need a version field in the parameter.

We can change this in the future with a new major release or add them as an optional thing

Yeah, the response should be exactly the same to do it in a minor release.

[Henrik-3]

As a reminder: The current version should be 100% compatible with the last update, so we kinda need a version field in the parameter.
We can change this in the future with a new major release or add them as an optional thing

Yeah, the response should be exactly the same to do it in a minor release.

I said that because of your idea with the method names, we can add them as an optional thing but should keep normal one without the version in it's name for now too

[raimannma]

As a reminder: The current version should be 100% compatible with the last update, so we kinda need a version field in the parameter.
We can change this in the future with a new major release or add them as an optional thing

Yeah, the response should be exactly the same to do it in a minor release.

I said that because of your idea with the method names, we can add them as an optional thing but should keep normal one without the version in it's name for now too

Yeah, I also support both in the python wrapper. As a method name or as parameter.

[jameslinimk]

Aight so I'll

• add an optional version parameter to all functions with multiple versions
• create new types and have it change the return type based on the version param
• create new tests for all the v1 functions
• Double check names as i belived i renamed 1 function because the orignal name was confusing

For leaderboard v2, I'll add new errors so it'll throw if any of the optional parameters are set and version is v2 as only v1 has functionality for those

Maybe keep the new functions and add deprecated functions to keep backwards compatibility?

I'm just making a checklist for myself here cause I'll work on this tommorow, I just came back from a flight

[jameslinimk]

Also @Henrik-3 I know it's pretty small of an issue, but some return types use different naming conventions. Like some will be camal cause while others snake case

[raimannma]

Also @Henrik-3 I know it's pretty small of an issue, but some return types use different naming conventions. Like some will be camal cause while others snake case

You mean raw & store endpoints are different to the other?

[jameslinimk]

Also @Henrik-3 I know it's pretty small of an issue, but some return types use different naming conventions. Like some will be camal cause while others snake case

You mean raw & store endpoints are different to the other?

Bro I stg I'm so tired. I thought when i was typing the endpoints I saw some camal case in non raw api stuff

Sorry

[raimannma]

Why ?

[jameslinimk]

I used _test for other things ill remove

[AMr4X]

force here can be undefined if it's not defined.

you can add a !! so if it's not defined

Suggested change

	async getAccount({ name, tag, force }: { name: string, tag: string, force?: boolean }): Promise<APIResponse<V1AccountResponse>> {
	this.validateArgs({ name, tag });
	return this.fetch<V1AccountResponse>(`v1/account/${name}/${tag}`, { force });
	}
	async getAccount({ name, tag, force }: { name: string, tag: string, force?: boolean }): Promise<APIResponse<V1AccountResponse>> {
	this.validateArgs({ name, tag });
	return this.fetch<V1AccountResponse>(`v1/account/${name}/${tag}`, { force: !!force });
	}

that way if it's not passed it will be false

[AMr4X]

same here

[jameslinimk]

The function used to deduce args from the object auto eliminates null values. Also default values dont work in deconstructed stuff

[Henrik-3]

Getting back to this: I try (!!!) to update this to v3 that is coming soon.
Never worked with TS, hopefully the shit i do doesn't look that bad (correct me if there is stuff that doesn't make sense) ^^