owengombas / discord.ts

🤖 Create your discord bot by using TypeScript and decorators!
https://owencalvin.github.io/discord.ts/
324 stars 40 forks source link

CommandMessage converts args to number without regards to whether it is safe to do so. (and ignores argtypes property) #18

Closed Adondriel closed 3 years ago

Adondriel commented 4 years ago

For an example: Discord_2020-07-17_223 The value being passed in, in this case is a roleid, roleid is supposed to be a string.

So, I told commandmessage that roleid should be a string. image

It still parsed it as a number. I fixed this locally as a test, in public/CommandMessage.js, in the parseArgs() function, by adding a check for || !Number.isSafeInteger(value) to the existing conditional

image

This fixed the issue locally.

However there seems to be a bigger issue here. It seems like it's not even checking the ArgsType interface that is passed in to the CommandMessage object... or I may not have found the proper documentation for this, as I can't find any docs on how to use CommandMessage with both ArgsType AND InfosType.

dbrxnds commented 3 years ago

I think that the best solution to this would be to just pass it on a string, and letting the consuming developer decide how to handle it?

AndyClausen commented 3 years ago

I took the liberty to create a PR for this (and used numberValue instead).

@Adondriel TypeScript types do not exist in runtime. This means that it cannot check the given type argument dynamically and decide what to do based on the type. So what you really want is another language where types exist on runtime.

@iDavidB I think it's okay to parse the numbers if they are actually valid numbers, since changing that now could be breaking. But yes, it probably should've just given the argument without any number parsing (especially when it does it silently) when it was first written 😛