The Wayback Machine - https://web.archive.org/web/20201023224831/https://github.com/discordjs/discord.js/pull/4869
Skip to content

/ discord.js

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

Jump to bottom

feat(MessageManager): extend API coverage #4869

Open
wants to merge 14 commits into
base: master
from
Open

feat(MessageManager): extend API coverage #4869

wants to merge 14 commits into from

Conversation

@monbrey
Copy link
Contributor

@monbrey monbrey commented Sep 30, 2020 •
edited

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.
monbrey added 9 commits Sep 14, 2020
@monbrey
feat(MessageManager): move Message#edit API calls to Manager
dfb2a20
@monbrey
fix(MessageEditOptions): update method reference
e9855f4
@monbrey
feat(MessageManager): move Message#crosspost API calls to Manager
909b7d7
@monbrey
feat(MessageManager): move Message#pin API calls to Manager
0afb292
@monbrey
feat(MessageManager): move Message#unpin API calls to Manager
ffb94ca
@monbrey
feat(MessageManager): move Message#react API calls to Manager
dcd5246
@monbrey
fix(MessageManager): throw TypeError on non-MessageResolvable params
ae2ffab
@monbrey
chore: typings
85646ca
@monbrey
Merge remote-tracking branch 'upstream/master' into message-manager-a…
Loading status checks…
c1e831c 
…dditions
@NotSugden
NotSugden reviewed Sep 30, 2020
View changes
Copy link
Contributor

@NotSugden NotSugden left a comment

just another nit, would it be better to use async functions and await the promises, rather than .then(() => this)?
src/managers/MessageManager.js Outdated Show resolved Hide resolved
@monbrey @NotSugden
fix(MessageManager): consistent API call notation
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1e84854 
Co-authored-by: Sugden <[email protected]>
@monbrey
Copy link
Contributor Author

@monbrey monbrey commented Sep 30, 2020

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.
@monbrey
chore: linting
Loading status checks…
96570d9
@monbrey monbrey mentioned this pull request Oct 1, 2020
Open
3 of 5 tasks complete
@izexi
izexi reviewed Oct 4, 2020
View changes
Copy link
Contributor

@izexi izexi left a comment

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.
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/managers/MessageManager.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@monbrey
Copy link
Contributor Author

@monbrey monbrey commented Oct 5, 2020

@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
Copy link
Contributor

@izexi izexi commented Oct 5, 2020

@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
advaith1 reviewed Oct 5, 2020
View changes
src/managers/MessageManager.js Outdated Show resolved Hide resolved
monbrey and others added 3 commits Oct 5, 2020
@monbrey @izexi @advaith1
fix: apply suggestions from code review
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fbf5f3c 
Co-authored-by: izexi <[email protected]>
Co-authored-by: Advaith <[email protected]>
@monbrey
fix: rename shadowed variable
Loading status checks…
d712d29
@monbrey
feat: typings for APIRawMessage overloads
Loading status checks…
4e49e04
@monbrey monbrey requested a review from kyranet Oct 16, 2020
@advaith1
Copy link
Contributor

@advaith1 advaith1 commented Oct 17, 2020

why isn't it imported from discord-api-types? iirc this was discussed in another pr but idr what happened
@monbrey
Copy link
Contributor Author

@monbrey monbrey commented Oct 17, 2020

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

I think because that's not a dependency of discord.js yet?
@vladfrangu
vladfrangu requested changes Oct 17, 2020
View changes
src/structures/Message.js
clone._patch(d);
return clone;
});
return this.channel.messages.edit(this.id, content, options).then(() => this);

This comment has been minimized.

Sign in to view
@vladfrangu

vladfrangu Oct 17, 2020
Member

The then will cause a different output than before (patched clone vs now the same message without the edits

This comment has been minimized.

Sign in to view
@monbrey

monbrey Oct 17, 2020
Author Contributor

Yeah I gotcha now, will resolve this.

typings/index.d.ts
@@ -2184,6 +2207,95 @@ declare module 'discord.js' {

type APIMessageContentResolvable = string | number | boolean | bigint | symbol | readonly StringResolvable[];

interface APIRawMessage {

This comment has been minimized.

Sign in to view
@vladfrangu

vladfrangu Oct 17, 2020
Member

This interface is incomplete.. why don't we use discord-api-types? I also don't remember the discussion for that

This comment has been minimized.

Sign in to view
@monbrey

monbrey Oct 17, 2020
Author Contributor

I'm happy to use it, just not sure if this PR should be covering that. I copied the interface from the PR izeki referenced in his comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@advaith1 advaith1

@NotSugden NotSugden

@izexi izexi

@vladfrangu vladfrangu

@kyranet kyranet

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
reason: API support semver: minor t: REST type: enhancement
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@monbrey @izexi @advaith1 @vladfrangu @kyranet @NotSugden @almostSouji
You can’t perform that action at this time.