feat(MessageManager): extend API coverage

[monbrey]

This is to be the first of a few PRs I plan to make which adds additional methods to the Manager classes. Open to discussion and feedback (and additional testing please!) before I start submitting others.

This allows users to directly handle not only creating and fetching Messages from the Manager, but also editing, crossposting, reacting to, pinning and unpinning messages by ID, allowing management of uncached messages.

Key to this is that this furthers the separation of API and cache management that was sought to be achieved by introducing Managers in the first place. However most operations still relied on the existing of a Structure, in cache, for any API methods to be called. This does not replace those methods, but moves the API-handling code into the Manager and calls it from the Structure.

MessageManager seemed like the logical place to start with this given it already provided support for deleting by ID, and is one of the two most common areas a structure is likely to be uncached.

Like the existing delete method, these all return Promise rather than returning the updated cached structure, as they're designed to compatible with uncached Messages.

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.

[NotSugden]

just another nit, would it be better to use async functions and await the promises, rather than .then(() => this)?

[monbrey]

just another nit, would it be better to use async functions and await the promises, rather than .then(() => this)?

Yeah I have no idea what's "better" here - I followed the convention which was already in place from the existing MessageManager#delete(). I have no preference for either and will take advice from maintainers.

[izexi]

On the Edit Message and Crosspost Message endpoints the message is returned as a response on success, it could be be useful for developers to have the according methods to return the raw message data or the Message itself. This would especially benefit those that don't have such messages cached and may need to access the data after calling the endpoint.

[monbrey]

@izexi Thanks for the review! Before I merge those suggestions though there's a few things to discuss.

• Is having inconsistent return values a good thing? I don't really have a side on that.
• Can the APIRawMessage data be used to cache a Message, and if so should it be in order to resolve the above inconsistency?
• Shouldn't we also have a jsdoc typedef for APIRawMessage rather than defining those as @returns {Message|Object}

[izexi]

@monbrey

• Is having inconsistent return values a good thing? I don't really have a side on that.

I don't really have a side on this too, I simply see it as making use of the data returned from the API which is would be a lot more useful than void to developers. If we're talking about the union return type, yes this may be a bit of a hassle for Typescript devs since they will need to add in that casting if they're going to make use of the returned value but I don't see this as a huge concern.

---

• Can the APIRawMessage data be used to cache a Message, and if so should it be in order to resolve the above inconsistency?

I agree that would be a good idea to solve the inconsistency, but I feel like that somewhat defeats the purpose of attempting to separate the cache from the managers if the data would just be cached within the method itself. Although its not very intuitive if the developer wanted to cache the message that wasn't already cached after calling the method from the manager, they could call MessageManager#add passing the returned raw message data to cache the Message.

---

• Shouldn't we also have a jsdoc typedef for APIRawMessage rather than defining those as @returns {Message|Object}

Yes, that would be ideal but in other places objects are just vaguely typed as Object in the JSDoc and it's going to be a bit of a pain to document a @typedef for the APIRawMessage interface since it has quite a lot of properties and nested properties so that might be a bit overkill.

[advaith1]

why isn't it imported from discord-api-types? iirc this was discussed in another pr but idr what happened

[monbrey]

why isn't it imported from discord-api-types?

I think because that's not a dependency of discord.js yet?

[iShibi]

Will MessageManager#reply be a good addition to this API coverage campaign? We can finally make some good use of that replyTo field in MessageOptions 😅

[advaith1]

you can just use Channel#send though?

[iShibi]

Yeah, totally forgot about that :) 👍

Stale

stale