Sapphire - Imagine a framework•2mo ago

weird behavior with args.*Result after updating to sapphire v5.3.1

Hello, I have this code (pls see screenshot) available as [p]base64 cmd, this same setup worked perfectly fine when I was on sapphire 5.2.1 but after updating to v.5.3.1 today weird issues like these are popping up. Did something change in @sapphire/result to introduce this change? because it feels like breaking. also I tried to check whats changed between @sapphire/result v2.6.6 ... v2.7.1 but it says 465 files changed and trying to browse it to figure out what changed makes chrome on my potato laptop hard crash, so can someone help me figure out around this issue? thanks.

Solution:

- if (getMode.isErr()) getMode = { value: 'decode' };
+ if (getMode.isErr()) getMode = ok('decode');

- if (getMode.isErr()) getMode = { value: 'decode' };
+ if (getMode.isErr()) getMode = ok('decode');

...

Jump to solution

27 Replies

zeroOP•2mo ago

for now I downgraded/reverted sapphire to v5.2.1 and @sappire/result to v2.6.6 which fixed the weird issue and cmds work again as usual

kyra•2mo ago

@owoer you're using a non-documented private field 😅 In 2.7.0 it changed to symbols to prevent accidentally accessing private fields However, fixing it is not hard! There are some patterns you're using that can be simplified :) For example getMode can be this:

const getModeResult = await args.pickResult('enum', { enum: nodes });
const getMode = getModeResult.unwrapOr('decode');

const getModeResult = await args.pickResult('enum', { enum: nodes });
const getMode = getModeResult.unwrapOr('decode');

Keep in mind that @sapphire/result is a library that has nearly all the features from Rust's Option<T> and Result<T, E>, which are very powerful on their own, so you should have everything you need without having to give up safety by accessing the raw data :)

zeroOP•2mo ago

hoh that's interesting 💀but what if getModeResult ends up in error, would it consume the error or propagate to user?

kyra•2mo ago

unwrapOr never throws Same for unwrapOrElse If you wanna check the source code: https://github.com/sapphiredev/utilities/blob/main/packages/result/src/lib/Result.ts And the changes in @sapphire/result v2.7.0: https://github.com/sapphiredev/utilities/pull/808

zeroOP•2mo ago

the thing is I'm not from rust background so I still find using both args.pick etc and args.pickResult bit difficult to use, there is lack of sections/examples for args.*Result in sapphirejs.dev guide area makes it difficult for someone not as proficient as me but as far as I remember from my early days here back when we had sapphire support in #old-sapphire-support , I used to struggle between which to choose between args.pick/rest vs args.*Result and the former never worked out for me due to so many ways I handle arguments in code, partly because lack of examples for it in the guide and partly because of my inability to understand how it works, so I had settled with args.*Result method since then and have used it everywhere because that feels more readable and fits with how I want to handle all sorts of arguments and atleast args.*Result was shown as one of the recommended ways back then in old support channel as rough examples from what I remember. now that I find out the said method I was using was non-documented private field and disallowed in 2.7.0, it kinda feels like semi breaking change and if this were mentioned in #Announcements it would have been greatly helpful to someone like me. To explain what I'm trying to say here, let me take the example you showed above:

const getModeResult = await args.pickResult('enum', { enum: nodes });

const getModeResult = await args.pickResult('enum', { enum: nodes });

what if a user inputs an invalid/unexpected/trollish argument here or let's say doesn't provide argument here, does it raise any error? if yes then do I check via isErr first then inform user? like how I'm doing currently or use args.pick(...).catch(() => ...) route? but that feels unpleasant UX wise, setting default can, at times, feel unwise than to parse/validate argument and inform user if the input doesn't match the expectation. Just trying to help you explain why I use args.*Result since so long. sorry for long wall so if I have to remain up-to-date with sapphire then I'll have to change this in so many places in 2+ years old code which feels huge pain and stressful :WhenDespairGetsAtYou: or I stay pinned on sapphire 5.2.1 forever and not have to deal with this unprepared change which kinda feels like breaking

kyra•2mo ago

I can understand the feeling, my main app, @Skyra, is ridden with legacy code that just repels me. When she was a multipurpose app back then, a single change, even technically non-breaking, was breaking Skyra. It felt like this comic all the time: https://xkcd.com/1172/

xkcd: Workflow

There are probably children out there holding down spacebar to stay warm in the winter! YOUR UPDATE MURDERS CHILDREN.

zeroOP•2mo ago

thank you for the kind words, it means a lot :sneakhug:

kyra•2mo ago

I also understand a lot of people don't have written Rust code. It is after all a very powerful but very complicated to learn and use language. Which is why I focused so much on the documentation, so you can see the methods it has and what you can do with them, also predict the behaviour it will have once you execute it (which to be fair is hard without Result). There are patterns you can follow, for example those codeblocks are similar:

const result = await promiseResultMethod();
const value = result.unwrapOr('test');

const result = await promiseResultMethod();
const value = result.unwrapOr('test');

const value = await promiseNativeMethod().catch(() => 'test');

const value = await promiseNativeMethod().catch(() => 'test');

let value;
try {
  value = resultMethod();
} catch {
  value = 'test';
}

let value;
try {
  value = resultMethod();
} catch {
  value = 'test';
}

Or even with Option:

const option = await promiseOptionMethod();
const value = option.unwrapOr('test');

const option = await promiseOptionMethod();
const value = option.unwrapOr('test');

const value = (await promiseOptionMethod()) ?? 'test';

const value = (await promiseOptionMethod()) ?? 'test';

zeroOP•2mo ago

sorry to say this 💀 but to my ADHD brain, that try-catch example makes the most sense because it feels more readable , in my head it went like "OHH OMG I can use it like this too? brooo this makes things so much easier to refactor 😭" I think I'll start refactoring my code slowly now, write a helper function to handle args then slap it in all 99 cmds then update to latest sapphire and see how it goes , thank you for all this help :luv: that try-catch also gives more flexibility on how I want to handle inputs from both success/failure pov so I guess that is why it feels the best choice for me :stolen_emoji:

kyra•2mo ago

The try-catch is precisely why we have Result, it's bulky otherwise. But like we support both handling your errors with Result, or with try-catch, it's your app, your code is up to you That being said, you made me realise I should add async variations of all the methods, so the following code becomes valid:

const mode = (await args.pickResult('enum', { enum: modes }))
  .unwrapOr('decode');

const text = (await (await args.restResult('string', { maximum: 1984, minimum: 2 }))
  .mapErrIntoAsync(() => {
    if (message.type === MessageType.Reply) {
      const ref = await message.fetchReference();
      return ok(ref.embeds.length > 0 ? ref.embeds[0].description : ref.content);
    }

    return err(new UserError('no input given, aborting...'));
  }))
  .unwrapRaw();

// ...

const mode = (await args.pickResult('enum', { enum: modes }))
  .unwrapOr('decode');

const text = (await (await args.restResult('string', { maximum: 1984, minimum: 2 }))
  .mapErrIntoAsync(() => {
    if (message.type === MessageType.Reply) {
      const ref = await message.fetchReference();
      return ok(ref.embeds.length > 0 ? ref.embeds[0].description : ref.content);
    }

    return err(new UserError('no input given, aborting...'));
  }))
  .unwrapRaw();

// ...

I really need to make AsyncResult and AsyncOption 😅

SirH•2mo ago

Yes, you could do isErr() if an input was invalid

kyra•2mo ago

All in all you can keep your code still, with minor changes

SirH•2mo ago

I noticed that particular part wasn't answered, so I was just filling in the gap

zeroOP•2mo ago

👍🏽

Solution

kyra•2mo ago

- if (getMode.isErr()) getMode = { value: 'decode' };
+ if (getMode.isErr()) getMode = ok('decode');

- if (getMode.isErr()) getMode = { value: 'decode' };
+ if (getMode.isErr()) getMode = ok('decode');

kyra•2mo ago

Same for getText And then on the last lines you do

const mode = getMode.unwrap();
let text = getText.unwrap();

const mode = getMode.unwrap();
let text = getText.unwrap();

zeroOP•2mo ago

ohhh I didn't know this can be simplified further like this :ablobmindblown: me rn

kyra•2mo ago

.unwrap() will throw an error if there's no value tho And needless to say, Sapphire comes with error handlers so UserError is sent to the user

zeroOP•2mo ago

yeah I was also thinking about that, I can handle that flexibly with try-catch 🙌

kyra•2mo ago

For example Skyra does this a lot:

const user = args.finished ? message.author : await args.pick('userName');
if (!user.avatar) this.error(LanguageKeys.Commands.Tools.AvatarNone);

const user = args.finished ? message.author : await args.pick('userName');
if (!user.avatar) this.error(LanguageKeys.Commands.Tools.AvatarNone);

And that'll send a translated message to the user using the guild's locale, using this code:

export function implementSkyraCommandError(identifier: string | UserError, context?: unknown): never {
    throw typeof identifier === 'string' ? new UserError({ identifier, context }) : identifier;
}

export function implementSkyraCommandError(identifier: string | UserError, context?: unknown): never {
    throw typeof identifier === 'string' ? new UserError({ identifier, context }) : identifier;
}

zeroOP•2mo ago

interesting 👀

kyra•2mo ago

And Sapphire args always throw an extension of UserError, so if you're not doing dynamic handling nor error message customization, you can do .pick instead

zeroOP•2mo ago

I have been meaning to extend UserError to customize the error messages that is more understandable to average discord users because with default hard-coded error messages like "There are no more arguments." the feedback I got is alot of users struggle to understand what they need to do, but I keep on forgetting and me coming from old commando js framework where users are still used to typing commands without having to pass any arguments and it sequentially asks the user to interactively input each argument one after other, I have some ideas to also have that type of args handling but that is probably offtopic to the OP so I apologise thank you for simplifying this for me with the amazing examples you have provided, it genuinely made the refactor so much less stressful, I would give you a hug if we were in IRL

kyra•2mo ago

By the way, the docs are a bit oddly formatted but, you can check all the methods Result has here: https://sapphirejs.dev/docs/Documentation/api-utilities/@sapphire/result/classes/Result/#methods

Sapphire Framework

Class: Result\ | Sapphire

A type used to express computations that can fail, it can be used for returning and propagating errors. This is a

zeroOP•2mo ago

actually I was reading that before opening this thread but the methods without examples went over my head 💀 so I said to myself lemme just ask in support forum to make it easier to understand for me 💀🙏🏽 and my dumb adhd brain can't make sense of all assert equal statements in examples

SirH•2mo ago

the assert equal is mainly a debugging/testing function to check if the outputted value from the left is equal to the value on the right

zeroOP•2mo ago

👍🏽 I would want to try contributing better examples if I get time or if my potato brain can formulate a clear visual of it or if it's needed

Gaming

Programming

weird behavior with args.*Result after updating to sapphire v5.3.1