Repository navigation
fix(plugin-gateway): keep the fetch calls that stay right on a cache miss - #235
Conversation
…miss Un-deprecate fetchHighest, fetchPermissions, fetchPermissionsIn and the fetchManageable/Kickable/Bannable/Moderatable family, and document on the getters that they skip roles missing from the cache.
|
Note This drawing shows
2 findings · reviewed 🟠
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one small README inconsistency inline.
Reviewed changes: the single commit on this PR (docs, JSDoc and one test; no runtime change).
- Un-deprecated fetch twins:
GuildMemberRoleManager#fetchHighestandGuildMember#fetchPermissions,fetchPermissionsIn,fetchManageable,fetchKickable,fetchBannable,fetchModeratablelose@deprecatedand gain@remarksexplaining they stay right on a cache miss. - Getter remarks:
roles.highest,roles.cache,permissions,permissionsIn,manageable,kickable,bannableandmoderatablenow say uncached roles are skipped without an error, with the fetch call as the fix. - README: the twins section lists the kept calls with an example.
- Test: evicts a member's only role and asserts the getters under-report while the seven fetch calls are correct.
- Changeset: patch for
@wolfstar/plugin-gateway.
I checked the new doc claims against the code: roles.fetch() only calls roles.fetchAll when the cached count differs from the member's role IDs, so "a complete cache makes no request" holds, and fetchPermissions/fetchPermissionsIn do fetch the guild, channel and roles. The test exercises the claimed behaviour.
claude-sonnet-5-5 ｜ 𝕏
@wolfstar/plugin-api
@wolfstar/plugin-broker
@wolfstar/plugin-cache
@wolfstar/plugin-gateway
@wolfstar/plugin-i18next
@wolfstar/plugin-logger
@wolfstar/plugin-scheduled-tasks
@wolfstar/plugin-sharder
@wolfstar/plugin-subcommands-advanced
commit: |
There was a problem hiding this comment.
ℹ️ No new issues in the latest commit; one earlier README note is still open.
Reviewed changes: the commit pushed since the prior Pullfrog review, which narrows the kept calls to the three named in the issue.
- Narrowed the un-deprecated set: only
GuildMemberRoleManager#fetchHighest,GuildMember#fetchPermissionsandGuildMember#fetchPermissionsInstay un-deprecated;fetchManageable,fetchKickable,fetchBannableandfetchModeratablereturn to their original deprecated JSDoc. - Repointed getter docs:
manageablenow recommendsawait member.roles.fetch()andme.roles.fetch(), or comparingfetchHighestof both;kickable,bannableandmoderatablejust say they skip uncached roles, likemanageable. - README and changeset: both now list the three kept calls and advise
roles.fetch()for the hierarchy getters. - Test: asserts the three kept calls are right on a cache miss, then
await top.roles.fetch()makesmanageablecorrect.sync-getters.test.tspasses locally (45 tests).
The README line 807 note from the first review (it names fetchPermissions() as a deprecated twin, although that one is now kept) is unchanged by this commit, so I left that thread open rather than repeating it.
claude-sonnet-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: the commit pushed since the prior Pullfrog review (docs only).
- README examples fixed: the twins paragraph now names only twins that stay deprecated (
fetchKickable(),fetchDeletable(),fetchEditable()), so "Most twins are deprecated" no longer contradicts the kept-calls paragraph below it. This resolves the earlier open README thread.
claude-sonnet-5-5 ｜ 𝕏
2 participants
🔗 Linked issue
Resolves #234
🧭 Context
With the
fetch*twins deprecated by #172, a role-hierarchy or permission check that must not under-report had no single call that was both correct and not deprecated.roles.highest,member.permissions,manageableand the like skip a role the cache lacks, without an error, so with a filtered cache or aplugin-brokerworker a lower-ranked member can pass a hierarchy check.📚 Description
Option 1 and option 3 from the issue: the single-call checks stay, and the getters now say they skip uncached roles. No runtime change.
Departures from the Pullfrog plan: it asked whether to keep just the three methods named in the issue or the four hierarchy checks too. I kept only the three named in the issue. The other twins, including
fetchManageable,fetchKickable,fetchBannableandfetchModeratable, stay deprecated as #172 decided; their getters now document that they skip uncached roles and point atroles.fetch(). Option 2 (a throwing getter /RoleUncached) is left out as the plan recommended; it would change discord.js parity and is better as an opt-in follow-up.Key changes
@deprecatedfromGuildMemberRoleManager#fetchHighest,GuildMember#fetchPermissionsand#fetchPermissionsIn, documented as the calls that stay right on a cache miss.@remarksonroles.highest,roles.cache,permissions,permissionsIn,manageable,kickable,bannableandmoderatablesaying uncached roles are skipped, with thefetch*call orawait member.roles.fetch()as the fix.highestis@everyone,manageableistrue) whilefetchHighest,fetchPermissionsandfetchPermissionsIngive the full answer.@wolfstar/plugin-gateway.Type of Change
Pre-flight Checklist
pnpm changeset(orpnpm changeset add --emptyif no release is needed). See .changeset/README.md.Verification:
pnpm build,pnpm typecheckandpnpm test(1681 tests) pass;oxlintreports no errors.pnpm lintfails locally onoxfmt --checkfor every one of the 549 files, because this Windows checkout has CRLF line endings; the commit hook formatted the staged files.