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

refactor!: consolidate parameters into a single options object #10718

Conversation

sdanialraza
Copy link
Contributor

@sdanialraza sdanialraza commented Jan 19, 2025

Please describe the changes this PR makes and why it should be merged:
Removes options as the second parameter. The first parameter now accepts id or options which can also have the id property. Internal discussion here

BREAKING CHANGE: ApplicationCommandManager#fetch and GuildApplicationCommandManager#fetch no longer accept 2 parameters. Instead, the first parameter accepts id or options which can now also have the id property.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

Copy link

vercel bot commented Jan 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Jan 25, 2025 8:35pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Jan 25, 2025 8:35pm

@sdanialraza sdanialraza force-pushed the refactor/consolidate-parameters-single-options-object branch 3 times, most recently from 9823d14 to f948dab Compare January 20, 2025 19:58
@sdanialraza sdanialraza marked this pull request as ready for review January 20, 2025 20:03
@sdanialraza sdanialraza requested a review from a team as a code owner January 20, 2025 20:03
Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

What about leaving the id overload (passing the id)? That is a common pattern for managers fetching.

@Jiralite Jiralite added this to the discord.js 15.0.0 milestone Jan 21, 2025
@sdanialraza sdanialraza force-pushed the refactor/consolidate-parameters-single-options-object branch from 6effd93 to 3dd35b1 Compare January 23, 2025 19:48
Copy link
Contributor

@OfficialSirH OfficialSirH left a comment

Choose a reason for hiding this comment

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

Muy bueno 👌

Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

I believe the breaking change entry needs amending. Would you mind doing that?

@sdanialraza
Copy link
Contributor Author

I believe the breaking change entry needs amending. Would you mind doing that?

Done

@sdanialraza sdanialraza force-pushed the refactor/consolidate-parameters-single-options-object branch from 162f21e to 59b7252 Compare January 25, 2025 19:50
@sdanialraza sdanialraza requested a review from almeidx January 25, 2025 19:51
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
@almeidx almeidx merged commit 695f592 into discordjs:main Jan 25, 2025
7 checks passed
@sdanialraza sdanialraza deleted the refactor/consolidate-parameters-single-options-object branch January 26, 2025 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants