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: allow discord.js builders to accept camelCase #7424

Merged
merged 6 commits into from
Feb 13, 2022

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Feb 7, 2022

Please describe the changes this PR makes and why it should be merged:

Closes #7376

Extends builder components in discord.js to allow both camelCase and snake_case inputs.

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 changes the library's interface (methods or parameters added)

@suneettipirneni suneettipirneni force-pushed the refactor/builder-camelcase branch 2 times, most recently from e5f454a to e43526a Compare February 7, 2022 16:51
@imranbarbhuiya
Copy link
Contributor

Why not embed too? also can we get that #equal in embed?

or can we get embedEqual, xComponentEqual in new component file?

@KhafraDev
Copy link
Contributor

I still don't see why this can't be left to the user 🤷

@suneettipirneni
Copy link
Member Author

I still don't see why this can't be left to the user 🤷

We had discussions about this internally, and we decided that this library shouldn't be that low level. We'd prefer to keep idiomatic js field names and have the library take care of the rest. The regular builders without json transformations are still available directly from the builders packages.

@KhafraDev
Copy link
Contributor

KhafraDev commented Feb 8, 2022

However it doesn't make much sense in this case, and leads to a bunch of other discussions about what the library should do. In this sense, snake_case is the only case accepted by Discord, so the data the user provides is the data that Discord receives.

edit: I recall a previous issue elsewhere where kyra noted that js doesn't have a universal style guide, so camelCase vs. snake_case vs PascalCase is up to the user to decide mostly, right? Why are only snake_case & camel case accepted in this pr?

@suneettipirneni
Copy link
Member Author

However it doesn't make much sense in this case, and leads to a bunch of other discussions about what the library should do. In this sense, snake_case is the only case accepted by Discord, so the data the user provides is the data that Discord receives.

I mean almost all of props in the library structures use camelcased naming schemes. Beyond classes a lot of pure json option objects are intentionally camelcased. So it would be inconsistent if message payload data didn't follow suit.

@KhafraDev
Copy link
Contributor

For the library it makes sense, but this is data that is being sent to Discord, which only accepts snake_case.

@suneettipirneni
Copy link
Member Author

For the library it makes sense, but this is data that is being sent to Discord, which only accepts snake_case.

It's sent on behalf of the user by the library. The library the developer never sees any of the actual endpoints like they would if the used a lower level library like /rest for example.

@monbrey
Copy link
Member

monbrey commented Feb 8, 2022

For the library it makes sense, but this is data that is being sent to Discord, which only accepts snake_case.

The data being sent to Discord is still snake_case, and isn't relevant to the changes being made here.

The standalone builders which exist under the /builders structures also haven't changed. They still only accept snake_case because they were designed to be used with /rest, to send data to Discord.

This PR extends those builder classes within the discord.js package to accept camelCase, which is consistent with the rest of discord.js and idiomatic JavaScript conventions.

up to the user to decide mostly, right

Up to the developer to decide. In this case, the developers of discord.js are free to decide on a convention. It's camelCase

@KhafraDev
Copy link
Contributor

and isn't relevant to the changes being made here.

It's entirely relevant... this change adds another layer of abstraction between the data a dev provides and the data sent to Discord. Not only this, but the behavior of discord.js is no longer uniform with builders with this change, which will definitely lead to confusion.

Up to the developer to decide. In this case, the developers of discord.js are free to decide on a convention. It's camelCase

and yet, both snake_case & camelCase are accepted here 😉. Theoretically, discord.js should only accept camelCase, since it's the convention throughout the rest of the lib, while builders uses snake_case.

@monbrey
Copy link
Member

monbrey commented Feb 8, 2022

the behavior of discord.js is no longer uniform with builders with this change

It never should have been. discord.js should be consistent with discord.js

discord.js should only accept camelCase

I agree, at least as far as the end user interface goes. But these classes are also used internally, when we receive snake_case data from Discord.

They always accepted both in the first place.

@suneettipirneni
Copy link
Member Author

Why not embed too? also can we get that #equal in embed?

or can we get embedEqual, xComponentEqual in new component file?

Support for embeds has been added, equals() method(s) will probably be added in another PR

@ImRodry
Copy link
Contributor

ImRodry commented Feb 10, 2022

Your commit was meant to say embeds not modals I imagine but why not this one? It’d be within the scope since we’re extending builders classes

@suneettipirneni
Copy link
Member Author

Your commit was meant to say embeds not modals I imagine but why not this one? It’d be within the scope since we’re extending builders classes

Mainly because the getters PR makes comparisons between components much easier.

@iCrawl iCrawl merged commit 94bf727 into discordjs:main Feb 13, 2022
@suneettipirneni suneettipirneni deleted the refactor/builder-camelcase branch July 6, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[discord.js@dev] Using JSON structures do not accept camelCase.