-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(*): enforce strings #4880
Conversation
21cc4b4
to
d65d8e2
Compare
Couldn't this just use the |
This comment has been minimized.
This comment has been minimized.
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
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. |
Next to that, shouldn't it use TypeError instead of RangeError? |
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. |
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix lints 🙏
eaa931d
to
6fc3e8c
Compare
@almostSouji rebase |
6fc3e8c
to
8562489
Compare
This needs a rebase. |
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
9e879dc
to
5182728
Compare
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
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 withverifyString
and let the context provide the error to be thrown in order to provide useful information (which embed or message attribute caused this error) utilizingRangeError
and the existing error message constant system.Input/Feedback requested
""
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
Semantic versioning classification: