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

feat(*): enforce strings #4880

Merged
merged 21 commits into from Jun 1, 2021
Merged

Conversation

almostSouji
Copy link
Member

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

This PR removes StringResolvable (which was just a nice naming scheme for "any" anyways) and enforces and tests for strings to be passed where before any could be resolved to strings.

We have a lot of issues in support regarding people wondering why certain parts of a message or embed may show "undefined" or "null" due to unexpectedly resolving them to strings (despite it being documented, as space has pointed out).

This is majorly opinionated and I personally believe we should hand the responsibility of providing message content as a string (and how it might be composed from the respective structures) up to the developer. While I do see that resolving anything to a string has its benefits (especially with structures that do have proper #toString implementations resulting in useful content), I also feel like this change will ultimately result in more verbose and explicit code for users as well as more useful pointers (as the previously resolved undefined -> "undefined", to name one example, would now throw with a useful error pointing clearly to the issue)

To achieve this I replaced the resolveString method with verifyString and let the context provide the error to be thrown in order to provide useful information (which embed or message attribute caused this error) utilizing RangeError and the existing error message constant system.

Input/Feedback requested

❓ For now this only throws with empty string where the API rejects. However while empty strings are valid (the API accepts and processes the request instead of throwing) in several positions in embed structures they do cause unexpected behavior. For example providing "" as embed author name or footer text will hide the respective icon, despite technically being valid (a zero-width space can circumvent this behavior). We could decide to be opinionated here and generally enforce a non-empty string to be passed where strings are expected in embeds.

Status

  • 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

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

src/errors/Messages.js Outdated Show resolved Hide resolved
@vaporoxx
Copy link
Contributor

vaporoxx commented Oct 3, 2020

Couldn't this just use the INVALID_TYPE error message instead of adding a new message for each MessageEmbed property?

@SpaceEEC SpaceEEC self-requested a review October 3, 2020 15:07
@almostSouji

This comment has been minimized.

@almostSouji
Copy link
Member Author

almostSouji commented Oct 6, 2020

Couldn't this just use the INVALID_TYPE error message instead of adding a new message for each MessageEmbed property?

I somehow missed we also resolve function input for this, one error message is variable location and must part is indeed sufficient.

Mark has brought up that we currently ignore if a function is passed in any URL field of embeds, which causes the handling further down the line to ignore the input. Explicit string casting makes use of discords validation and results in

DiscordAPIError: Invalid Form Body
embed.author.icon_url: Not a well formed URL.

Applying this seems to fit nicely into the scope of this PR to more strictly enforce strings (and valid input) rather than defaulting to still valid embeds being sent.

src/errors/Messages.js Outdated Show resolved Hide resolved
@vaporoxx
Copy link
Contributor

vaporoxx commented Oct 6, 2020

Next to that, shouldn't it use TypeError instead of RangeError?

@almostSouji
Copy link
Member Author

Considered that as well, truth be told I by now have lost any and all intuition as to when which of the two is used. Apparently non-empty string is range but only string would be Type?

In which case it might even make sense to dynamically instantiate the error in the method and just pass the string? I really don't know anymore.

@almostSouji
Copy link
Member Author

Re-viewing API behavior it might even be feasible to default to empty strings if a non-string is provided and let the API handle the rest where non-empty field input is required.

Typing wise this won't happen in TS. However justifying type checks in only select areas of the library consistency wise seems to be a bad idea. If type checks are wanted one should really write TS instead of JS.

Casting (not resolving) to string here might be the correct decision following this mindset, reducing this to removal of resolveString and typings (where strings can quite easily be enforced).

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Fix lints 🙏

src/util/Util.js Outdated Show resolved Hide resolved
@almostSouji almostSouji force-pushed the maj/strict-strings branch 6 times, most recently from eaa931d to 6fc3e8c Compare November 25, 2020 16:44
@iCrawl
Copy link
Member

iCrawl commented Dec 14, 2020

@almostSouji rebase

src/errors/Messages.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/MessageEmbed.js Outdated Show resolved Hide resolved
src/structures/APIMessage.js Outdated Show resolved Hide resolved
@iCrawl iCrawl requested a review from SpaceEEC April 3, 2021 13:17
@iCrawl
Copy link
Member

iCrawl commented Apr 3, 2021

This needs a rebase.

typings/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
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.

None yet

7 participants