SIA
Sapphire - Imagine a framework•16mo ago
some1chan

Why in line 79 for src/preconditions/ClientPermissions.ts is there `PermissionsBitField | null`?

Hello, I'm doing some (potentially bad, please warn me) things to be able to check permissions whenever I need it in my bot. I figured to have sane code, I'd copy off of Sapphire's precondition code for myself to use, specifically ClientPermissions. As far as I know, if I want localization and sane control over the message text when there's a permission error, and the permission check can be done anyway, I'm guessing this is how I do it. While looking through the code, I noticed the explicit type cast PermissionsBitField | null. I also noticed this could be avoided, assuming the types weren't lying to me from channel.permissionFor(me). Upon checking the commit for more info, it said it fixed permission checking for old applications. I can't find any more info besides that, and the pull request attached to it. Would anyone from the dev team be able to shine any light on it? (i'd ping favna judging by the commit author but that seems impolite) Commit in question: https://github.com/sapphiredev/framework/commit/725a7d3a8e8f92860aaf946aecfc988abc8d9553
17 Replies
some1chan
some1chanOP•16mo ago
private async getPermissionsForChannel(channel: TextBasedChannel, messageOrInteraction: Message | BaseInteraction) {
    // Wondering about this line
    let permissions: PermissionsBitField | null = this.dmChannelPermissions;

    if (messageOrInteraction.inGuild() && !channel.isDMBased()) {
        if (!isNullish(messageOrInteraction.applicationId)) {
            permissions = channel.permissionsFor(messageOrInteraction.applicationId);
        }

        if (isNullish(permissions)) {
            const me = await messageOrInteraction.guild?.members.fetchMe();
            if (me) {
                permissions = channel.permissionsFor(me);
            }
        }
    }

    return permissions;
}
private async getPermissionsForChannel(channel: TextBasedChannel, messageOrInteraction: Message | BaseInteraction) {
    // Wondering about this line
    let permissions: PermissionsBitField | null = this.dmChannelPermissions;

    if (messageOrInteraction.inGuild() && !channel.isDMBased()) {
        if (!isNullish(messageOrInteraction.applicationId)) {
            permissions = channel.permissionsFor(messageOrInteraction.applicationId);
        }

        if (isNullish(permissions)) {
            const me = await messageOrInteraction.guild?.members.fetchMe();
            if (me) {
                permissions = channel.permissionsFor(me);
            }
        }
    }

    return permissions;
}
My potential code, in which I'm naively unaware if I'm about to make a mistake: 
// Also pulled from the same src/preconditions:
const dmChannelPermissions = new PermissionsBitField(
    ~new PermissionsBitField([
        //
        PermissionFlagsBits.AddReactions,
        PermissionFlagsBits.AttachFiles,
        PermissionFlagsBits.EmbedLinks,
        PermissionFlagsBits.ReadMessageHistory,
        PermissionFlagsBits.SendMessages,
        PermissionFlagsBits.UseExternalEmojis,
        PermissionFlagsBits.ViewChannel,
    ]).bitfield & PermissionsBitField.All,
).freeze();

export async function getPermissionsForChannel(
    channel: TextBasedChannel,
    messageOrInteraction: Message | BaseInteraction,
) {
    // Edited to just use the dmChannelPermissions type, which from the types and my own understanding will always be set.
    let permissions = dmChannelPermissions;

    if (messageOrInteraction.inGuild() && !channel.isDMBased()) {
        let newPermissions: PermissionsBitField | null = permissions;
        if (!isNullish(messageOrInteraction.applicationId)) {
            newPermissions = channel.permissionsFor(
                messageOrInteraction.applicationId,
            );
        }

        if (isNullish(newPermissions)) {
            const me = await messageOrInteraction.guild?.members.fetchMe();
            if (me) {
                permissions = channel.permissionsFor(me);
            }
        } else {
            permissions = newPermissions;
        }
    }

    return permissions;
}
// Also pulled from the same src/preconditions:
const dmChannelPermissions = new PermissionsBitField(
    ~new PermissionsBitField([
        //
        PermissionFlagsBits.AddReactions,
        PermissionFlagsBits.AttachFiles,
        PermissionFlagsBits.EmbedLinks,
        PermissionFlagsBits.ReadMessageHistory,
        PermissionFlagsBits.SendMessages,
        PermissionFlagsBits.UseExternalEmojis,
        PermissionFlagsBits.ViewChannel,
    ]).bitfield & PermissionsBitField.All,
).freeze();

export async function getPermissionsForChannel(
    channel: TextBasedChannel,
    messageOrInteraction: Message | BaseInteraction,
) {
    // Edited to just use the dmChannelPermissions type, which from the types and my own understanding will always be set.
    let permissions = dmChannelPermissions;

    if (messageOrInteraction.inGuild() && !channel.isDMBased()) {
        let newPermissions: PermissionsBitField | null = permissions;
        if (!isNullish(messageOrInteraction.applicationId)) {
            newPermissions = channel.permissionsFor(
                messageOrInteraction.applicationId,
            );
        }

        if (isNullish(newPermissions)) {
            const me = await messageOrInteraction.guild?.members.fetchMe();
            if (me) {
                permissions = channel.permissionsFor(me);
            }
        } else {
            permissions = newPermissions;
        }
    }

    return permissions;
}
Lioness100
Lioness100•16mo ago
It looks like channel.permissionsFor() is only nullable in some cases. In the typings, there is an override where it is nullable (if you pass in a role or member resolvable, because it doesn't know if it actually exists), and an override where it isn't (if you pass the role or member itself) fetchMe() doesn't return a resolvable, though, so I think you're right in that the | null is unnecessary Oh, nevermind It could also be the return type of permissionsFor where the application id is passed, which is a resolvable, and therefore the result is nullable. So, in conclusion, the | null is added to allow: 
            newPermissions = channel.permissionsFor(
                messageOrInteraction.applicationId,
            );
            newPermissions = channel.permissionsFor(
                messageOrInteraction.applicationId,
            );
some1chan
some1chanOP•16mo ago
I was about to post this:
in my edit, i spun off permissions into a new variable newPermissions. This is so I could do channel.permissionsFor(messageOrInteraction.applicationId); without the potential null output affecting the type of the function getPermissionsForChannel(), which I understand to be unnecessary. I think getPermissionsForChannel() can just be of type PermissionsBitField and not PermissionsBitField | null. If this was ever turned null by the applicationId fetch, I believe it'd make sure it'd always not be null, due to fetchMe(), presumably because fetchMe() would never be null
...except permissions is still technically nullable, because if fetchMe() for some reason can be undefined. I'm not 100% sure on how that'd be the case, but I presume that makes the | null function typing make sense. Did I get that right? (also, the reason on why it'd be undefined would be great. best guess is that somehow d.js has http bot support or something)
Lioness100
Lioness100•16mo ago
From the docs, I don't think fetchMe() can be undefined. However, me would be undefined if messageOrInteraction.guild is.
some1chan
some1chanOP•16mo ago
oh wait a sec im getting undefined because of messageOrInteraction.guild?, yep in that case, how can we be in a guild and have the guild be null?
Lioness100
Lioness100•16mo ago
Use inCachedGuild() instead of inGuild() inGuild() will only ensure guildId, not guild
some1chan
some1chanOP•16mo ago
inCachedGuild() doesn't seem to work due to the Message typing, hmm
Lioness100
Lioness100•16mo ago
You could have it be Message<true> | BaseInteraction<'cached'> in the parameter types, and force the caller to do the typeguards specific to their context beforehand? Or you could just use the non-null assertion operator Or you could use channel.guild instead, if that's not nullable
some1chan
some1chanOP•16mo ago
oh hey yeah, i can guarantee channel being a non-partial also! i think that's the best solution there
Lioness100
Lioness100•16mo ago
cool beans 😎
some1chan
some1chanOP•16mo ago
My current guess for the original question is that the null typing is not needed then. Even if channel is a partial, and guild is unavailable, it'd be a DM channel judging by types. Might be worth refactoring later, unless a contributor who actually knows the codebase says otherwise. See https://discord.com/channels/737141877803057244/1150945163473059950/1151019618727645205 also wondering if this convo should've been in #framework-development but it did have potential branches of "use this part of the framework and don't do what horrible thing you're doing"
Lioness100
Lioness100•16mo ago
Wdym by this? It's null because the guild might not be cached
some1chan
some1chanOP•16mo ago
i'm guessing guild will always be cached, solely because of d.js's typings, since guild in channel.guild after !channel.isDMBased() can't be null. I could be easily wrong, as i don't know d.js's internals at all
Lioness100
Lioness100•16mo ago
There are edge cases where it wouldn't be there, so sapphire's just playing it safe 🙂
some1chan
some1chanOP•16mo ago
oh fun what's the edge cases?
Favna
Favna•16mo ago
If you configure it not to be in your client options or if you're sharding and the shard to that guild has gone down
some1chan
some1chanOP•16mo ago
ah, that makes sense. thanks for the reply!

Did you find this page helpful?