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: switch xURL to xUrl #7388

Closed
wants to merge 1 commit into from

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Feb 2, 2022

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

Bringing back the discussion from #6036! But why?!? This change has been made under a number of factors:

  1. A snakeCase function converts iconURL as icon_u_r_l, so if we want to support such utility seamlessly, we would need to pass iconUrl. In this case, a name casing mismatch happens as the property is iconURL inside the builder methods while icon_url in the constructor. For consistency, we can use and adopt iconUrl, this way it's the same casing for both the utility function and the chainable methods.

    This means that this code is now possible:

    new Embed(snakeCase({
      description: 'Some description',
      footer: {
        text: 'Hello there',
        iconUrl: user.displayAvatarUrl(),
      },
    }));

    Or alternatively:

    new Embed({
      description: 'Some description',
      footer: snakeCase({
        text: 'Hello there',
        iconUrl: user.displayAvatarUrl(),
      }),
    });
  2. There are several things in the JavaScript ecosystem which uses Url, mostly from Firefox but also supported in other places:

    So xUrl isn't that unknown in JavaScript, despite URL and URLSearchParams existing.

  3. We are switching to TypeScript soon, which is a language created by the creators of C#, and as such, they share a lot of the naming conventions (see types, interfaces, enums, and namespaces) on top of what they already shared (PascalCase for classes, camelCase for local variables). However, something JavaScript falls short on is having a proper and solid set of rules that define whether or not names should be capitalized or not. Fortunately, C# does, so we can take advantage of this to provide a consistent name casing, as seen below:

    When using acronyms, use Pascal case or camel case for acronyms more than two characters long. For example, use HtmlButton or htmlButton. However, you should capitalize acronyms that consist of only two characters, such as System.IO instead of System.Io.
    From: https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-1.1/141e06ef(v=vs.71)?redirectedfrom=MSDN

  4. Easier to read, now this is subjective, but let me bring some examples:

    • CreateGuildScheduledEventInviteURLOptions -> CreateGuildScheduledEventInviteUrlOptions
    • ImageURLOptions -> ImageUrlOptions
    • StaticImageURLOptions -> StaticImageUrlOptions
    • bannerURL() -> bannerUrl()
    • splashURL() -> splashUrl()
    • iconURL() -> iconUrl()

As a standalone word, we can still use URL (aka uppercase), but if it's joined with other words, I'd argue we should go for .NET's rules and replacing PascalCase to camelCase where applicable. After all, they are pretty solid and promote consistency in a way that's widely recognized by many developers and is free of doubts.

After all, we used .NET's rules alongside JS's adoption of Id to approve the aforementioned PR.

I'll also update discord-api-types to reflect the naming change from this PR in a bit! Done! discordjs/discord-api-types#313

Status and versioning classification:

  • 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)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@mdashlw
Copy link

mdashlw commented Feb 2, 2022

What about DMChannel/createDM/isGroupDM/others, setRTCRegion, setNSFW, setAFKChannel and probably more? Enums from types like GuildMFALevel? And the obvious REST[...] and API[...] also from types

@kyranet
Copy link
Member Author

kyranet commented Feb 2, 2022

DM will stay as-is (see the quote from MSFT's docs). RTC, NSFW, AFK, MFA, REST, and API will need renaming, yes, since they're more than 2 characters long.

I'll bring this internally and discuss the scope expansion with the rest of the team, I mostly wanted to do those changes incrementally to avoid creating many merge conflicts.

@ImRodry
Copy link
Contributor

ImRodry commented Feb 2, 2022

Not gonna touch on the other arguments since they all seem valid to me and I have nothing against them. This isn't something I disagree with necessarily either, just wanna point out that the first argument isn't really valid:

A snakeCase function converts iconURL as icon_u_r_l, so if we want to support such utility seamlessly, we would need to pass iconUrl. In this case, a name casing mismatch happens as the property is iconURL inside the builder methods while icon_url in the constructor. For consistency, we can use and adopt iconUrl, this way it's the same casing for both the utility function and the chainable methods.

image

@KhafraDev
Copy link
Contributor

KhafraDev commented Feb 2, 2022

Not gonna touch on the other arguments since they all seem valid to me and I have nothing against them.

None of the points listed are actually valid though:

  1. As Rodry mentioned, a badly implemented snakeCase might not format the values correctly, but it's very easy to do correctly.
  2. The "several places" mentioned are mostly all APIs for browser extensions and the one other place is due to it being part of the WebDriver BiDi spec! As kyra mentioned, both URL and URLSearchParams are places where URL isn't Url.
  3. Neither TypeScript nor Javascript are C#, so I don't understand the want to adopt a C# styling guidelines. Plus, as mentioned before, URL and URLSearchParams are both capitalized which should show conformity in js naming standards. Let's not forget about URLPattern either, a new proposal that's already being shipped in Chrome 95+ either...
  4. As mentioned, subjective.

@kyranet
Copy link
Member Author

kyranet commented Feb 2, 2022

Apparently lodash's snakeCase function comes with a correctly implemented function, so this is not needed.

@kyranet kyranet closed this Feb 2, 2022
@kyranet kyranet deleted the refactor/switch-to-url branch February 2, 2022 20:21
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.

5 participants