Discord API#191
Conversation
So comparisons like `DiscordApi.currentUser === DiscordApi.currentUser` can be true
|
Aside from the snake_case being there (especially since it's being intermixed with camelCase), I would approve this. It's pretty close to the restructure I had planned for the next version of the API anyways and takes quite a few things to add off my list. |
zerebos
left a comment
There was a problem hiding this comment.
Overall I think this restructure is nice, as it gives more explicit types to objects. There were a couple errors, and some naming convention issues that should be addressed however.
| async ensurePrivateChannel() { | ||
| if (DiscordApi.currentUser === this) | ||
| throw new Error('Cannot create a direct message channel to the current user.'); | ||
| return Channel.from(await Modules.PrivateChannelActions.ensurePrivateChannel(DiscordApi.currentUser.id, this.id)); |
There was a problem hiding this comment.
This should use Channel.fromId since ensurePrivateChannel returns the ID of the channel
| /** | ||
| * Whether this channel is the guild's default channel. | ||
| */ | ||
| get default_channel() { |
There was a problem hiding this comment.
boolean return should have affirmative or "is" prefix. default_channel should be isDefaultChannel
| let response = {}; | ||
| if (parse) response = await Modules.MessageActions._sendMessage(this.id, Modules.MessageParser.parse(this.discordObject, content)); | ||
| else response = await Modules.MessageActions._sendMessage(this.id, {content}); | ||
| return Message.from(Modules.MessageStore.getMessage(this.id, response.body.id)); |
There was a problem hiding this comment.
Looks like it returns Message but you have it labelled as returning Promise
There was a problem hiding this comment.
This is an async function so the actual return value is a promise. I'll add that it resolves to a message.
There was a problem hiding this comment.
Fair point, actual return is the Promise, but I think it's best we say what it resolves to especially since it should always resolve to Message type
| static get GroupChannel() { return GroupChannel } | ||
|
|
||
| get id() { return this.discordObject.id } | ||
| get application_id() { return this.discordObject.application_id } |
There was a problem hiding this comment.
I'll make this comment once, but it applies throughout all these changes. You have a mix of snake_case and camelCase here and the codebase we have already has been going with camelCase although of course we have consistencies in the codebase itself but I think it's better to make sure we consistently use camelCase. For reference look at the similar structs done here: https://github.com/JsSucks/BetterDiscordApp/tree/master/client/src/structs/socketstructs
I can tell you were trying to use snake_case for attributes but it wasn't done consistently and it feels weird to have two quite different naming schemes happening at the same time.
There was a problem hiding this comment.
I forgot about the socket structs. I tried to normalise attribute names to snake case as Discord was using them inconsistently. I'll change these to camel case then.
| /** | ||
| * Whether this channel is currently selected. | ||
| */ | ||
| get selected() { |
There was a problem hiding this comment.
We should probably change this to isSelected
| /** | ||
| * Whether this channel is currently selected. | ||
| */ | ||
| get selected() { |
There was a problem hiding this comment.
Like the other one we should probably rename to isSelected
|
|
||
| /** | ||
| * Deletes the message. | ||
| * TODO: how do we know if the message was deleted successfully? |
There was a problem hiding this comment.
We can do a temporary subscribe to the message delete event
There was a problem hiding this comment.
That would tell us if the message was deleted successfully, but won't help if it isn't. We would only be able to tell if it failed if a timeout passes and we wouldn't be able to tell if an error had been returned.
There was a problem hiding this comment.
Internally it does attempt to retry for awhile if there are errors and then fire the event when it completes. Otherwise we could copy the internals of the deleteMessage function but add in passing the error callback to the delete() request
| * Deletes the message. | ||
| * TODO: how do we know if the message was deleted successfully? | ||
| */ | ||
| delete() { |
There was a problem hiding this comment.
We should probably add a permission check similar to edit message here
| return Modules.UserStatusStore.getActivity(this.id); | ||
| } | ||
|
|
||
| get direct_messages() { |
There was a problem hiding this comment.
name here is unclear, makes it seem like it would return a list of all the DMs
There was a problem hiding this comment.
I've renamed this to privateChannel.
| Modules.MessageActions.startEditMessage(this.channel_id, this.id, this.content); | ||
| } | ||
| static get modules() { return Modules } | ||
| static get User() { return User } |
There was a problem hiding this comment.
Just curious, is the point of having all the types as static members a way for plugins to check types of objects?
There was a problem hiding this comment.
I originally added it so you could use static functions like Channel.fromId where you had a channel's ID instead of getting all channels. I didn't actually think about that.
There was a problem hiding this comment.
Ah alright, that works as well, I didn't think about it that way either. Personally I have been doing api.channels.find(c => c.id == "XXXXXXXXXX") 😀
…icates when multiple modals are opened at the same time
# Conflicts: # client/src/modules/pluginapi.js # client/src/ui/bdui.js # client/src/ui/dom.js # client/src/ui/ui.js # package-lock.json # package.json
|
|
Re: The weakmaps/sets it goes against the point of them because the references are supposed to be weak and allow garbage collection, what you were doing introduces non-determinism. |
Additional changes not related to the Discord API: