Skip to content

Conversation

@null8626
Copy link
Member

@null8626 null8626 commented Sep 12, 2025

The following pull request adapts the SDK to API v1.

): Promise<any> {
const headers: IncomingHttpHeaders = {};
if (this.options.token) headers["authorization"] = this.options.token;
if (this.options.token) headers["authorization"] = `Bearer ${this.options.token}`;
Copy link

Choose a reason for hiding this comment

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

Would also change the current v0 implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh v0 does not work with Bearer tokens? Damn

Copy link

Choose a reason for hiding this comment

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

Why is this resolved?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I don't remember. My bad. Resolved.

@null8626
Copy link
Member Author

null8626 commented Sep 18, 2025

Breaking changes reverted.

Full diff between this and the commit prior to #91: 24a2ee1...null8626:node-sdk:master

Tested with npm run test. All that's left now is updating the README.

@jpbberry
Copy link
Member

jpbberry commented Sep 18, 2025

Not sure I agree with the concept of a separate class to denote a changing of versions. Either we should fit methods into the old class definition or we bump a major version.
Is there a discussion on this that I missed?

@null8626
Copy link
Member Author

null8626 commented Sep 18, 2025

Veld explicitly proposed that newer endpoints use a separate v1 class that inherit the older one. I believe he wanted this so that developers have a clear distinction on which is a v1 endpoint and which is a legacy endpoint. For example legacy's hasVoted can be confused with v1's getVote because they serve similar a purpose. I originally planned for the hasVoted method to use the v1 equivalent for it, but it would be a breaking change, and Veld is opposed to breaking changes that would require a major update in this situation (probably because if it's a major update, developers might miss it because they set the dependency versioning of @top-gg/sdk in their package.json to ^3.1.0 or whatever.)

Not only that, this is also supported by the fact that the v1 endpoints require a newer API token, while the legacy endpoints work with both legacy and newer API tokens.

@jpbberry
Copy link
Member

jpbberry commented Sep 18, 2025

@velddev mind weighing in on this? What's the aversion to a breaking major version bump? "Missing an update" doesn't really matter if things aren't changed to not break.

E.g. most of the old methods still use the v0 endpoints, so if v0 gets shut off it won't matter if people updated to the new package version or not.
Even people who go out of their way to implement the ApiV1 will also start failing when v0 stops.

I don't see any advantage for shying away from a breaking version here, and seems to break the line between service and library to change how the client is interacted with for an API change.

@velddev
Copy link

velddev commented Sep 18, 2025

The goal of these changes is to build the foundation for the v1 API. The legacy API is finished. Any change we introduce shouldn't cause users who use the v0 API to have to change their code and make the experience adding new functionality more cumbersome.

A breaking change would dictate that we edit the current schema, and as long as the current API does not do that, we will not need to do this to the end user.

What we instead should do is have a new client which is built for the v0 API so that the end user can gradually adopt new features without the weight of having to migrate their current code to a new arbitrary refactored client version.

@velddev
Copy link

velddev commented Sep 18, 2025

An example: how many times have you had to change your code because a library did a breaking change? Was it a good experience having to do that?

Now imagine that the breaking change does not give you any benefits.

Refactoring for the sake of it, while great for our code maintainability, is selfish towards the end user since every hour that is spent on refactoring, every user of the library will spend an equal amount of time figuring out how to fix their code

@jpbberry
Copy link
Member

I think the disconnect comes from the fact that we're transitioning to a V1 API without it being able to expose the same functionality as V0.

I imagine this will add more refactoring for users than not. But if the V1 isn't going to able to do what V0 does then it is what it is.

@null8626
Copy link
Member Author

null8626 commented Oct 3, 2025

Breaking changes reverted.

Full diff between this and the commit prior to #91: 24a2ee1...null8626:node-sdk:master

Tested with npm run test. All that's left now is updating the README.

README updated.

@null8626 null8626 force-pushed the master branch 2 times, most recently from ab74a42 to bba940b Compare October 10, 2025 07:04
Comment on lines +304 to +314
* ```js
* // Discord.js:
* const commands = (await bot.application.commands.fetch()).map(cmd => cmd.toJSON());
*
* // Eris:
* const commands = await bot.getCommands();
*
* // Discordeno:
* import { getApplicationCommands } from "discordeno";
*
* const commands = await getApplicationCommands(bot);
Copy link

Choose a reason for hiding this comment

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

these docs are awesome, thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, Veld! ❤️

velddev
velddev previously approved these changes Feb 1, 2026
Copy link

@velddev velddev left a comment

Choose a reason for hiding this comment

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

Assuming based on the PR, the two clients are split. LGTM.

/** Possible response from Request */
public response?: Dispatcher.ResponseData;
constructor(code: number, text: string, response: Dispatcher.ResponseData) {
export default class APIError extends Error {
Copy link
Member

Choose a reason for hiding this comment

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

pretty sure this would be a breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. Resolved.

Copy link
Member

Choose a reason for hiding this comment

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

The response object is still missing, it's an accessible field

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will revert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted.

@null8626 null8626 requested a review from jpbberry February 1, 2026 21:42
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.

3 participants