Sapphire - Imagine a framework•9mo ago

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•9mo 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•9mo 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•9mo 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•9mo ago

From the docs, I don't think fetchMe() can be undefined. However, me would be undefined if messageOrInteraction.guild is.

some1chan•9mo 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•9mo ago

Use inCachedGuild() instead of inGuild() inGuild() will only ensure guildId, not guild

some1chan•9mo ago

inCachedGuild() doesn't seem to work due to the Message typing, hmm

Lioness100•9mo 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•9mo ago

oh hey yeah, i can guarantee channel being a non-partial also! i think that's the best solution there

Lioness100•9mo ago

cool beans 😎

some1chan•9mo 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•9mo ago

Wdym by this? It's null because the guild might not be cached

some1chan•9mo 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•9mo ago

There are edge cases where it wouldn't be there, so sapphire's just playing it safe 🙂

some1chan•9mo ago

oh fun what's the edge cases?

Favna•9mo 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•9mo ago

ah, that makes sense. thanks for the reply!

Gaming

Programming

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