NanderTGA / msgroom-orm

A MsgRoom client.
https://nandertga.github.io/msgroom-orm/
MIT License
4 stars 0 forks source link

Classes instead of objects for commands #124

Closed mybearworld closed 9 months ago

mybearworld commented 9 months ago

Say you wanted to implement a counter command. It'd look like this:

let counter = 0;
client.commands.counter = {
  description: "A counter.",
  handler: () => `The counter is ${++counter}!`,
}

This works, but it uses a global variable, so it pollutes the global scope.

A much nicer way to do this would be:

client.commands.counter = class {
  private counter = 0;

  public description = "A counter.";
  public handler() {
    return `The counter is ${++this.counter}`;
  }
};

I think this would improve the UX of this library by a lot. I'm currently facing this problem with my toB's command:

const hangmanSymbols = [
  // ...
];
let hangman: string | null = null;
let hangmanIncorrectGuesses = 0;
let hangmanGuessed: string[] = [];
const hangmanGuessesAmount = 9;
const hangmanReset = () => {
  // ...
};
const hangmanView = () => {
  // ...
};
const hangmanSolved = () => {
  // ...
};
client.commands.hangman = {
  // ...
};
NanderTGA commented 9 months ago

Interesting suggestion. I have some comments though.

I should also note here that the underlying problem is state management, which on one side sounds nice to solve but on the other side that would take a lot of work for something that can already be solved in an easier way.

The only thing I'm thinking I could implement is to instantiate a class automatically when a command is found to be a class. Not sure how I'd do that though, so that's unlikely as well.

I suggest you look into yabluzo's code to get an example on how modules work.

mybearworld commented 9 months ago
  • You can split toB up into multiple files using modules (or import the commands manually from another file)

Creating a separate file for every command that has state would not be ideal, though.

  • you can technically use classes with static properties, the only issue with endorsing this are breaking changes every time I add new metadata. The solution would be to tell people to use private class variables (by prefixing the name with an #, this is a language feature in JS)

You're not able to do much with state with static-only classes.

The only thing I'm thinking I could implement is to instantiate a class automatically when a command is found to be a class. Not sure how I'd do that though, so that's unlikely as well.

typeof foo === "function"?

NanderTGA commented 9 months ago

Creating a separate file for every command that has state would not be ideal, though.

My idea is to create a file for every group of commands.

You're not able to do much with state with static-only classes.

You can perfectly use static properties or use an immediately constructed class (or whatever the name of an IIFE but with a class is).

typeof foo === "function"?

The main problem with the automatic instantiating of classes would be how I would do this. Detecting that it's a class is not a problem but where do I keep the instance? What if the class changes? I'm sorry but that would be too much of a headache.

mybearworld commented 9 months ago

You can perfectly use static properties or use an immediately constructed class (or whatever the name of an IIFE but with a class is).

I tried that, and it didn't work. The handler was called without the correct this.

where do I keep the instance? What if the class changes?

Just storing it in the same place as the other commands wouldn't work? And what do you mean by the class changing?

NanderTGA commented 9 months ago

You can perfectly use static properties or use an immediately constructed class (or whatever the name of an IIFE but with a class is).

I tried that, and it didn't work. The handler was called without the correct this.

Sorry for the late reply but this is an easy fix. I'll fix this soon.

where do I keep the instance? What if the class changes?

Just storing it in the same place as the other commands wouldn't work? And what do you mean by the class changing?

Storing the class instance is not really the problem (I'd probably use Symbol for this). The thing is it would need to change when you redefine the command to be another class or other things that might change the class or whatever. If that happens how would I know? Using getters and setters here is going to be too hard. Should the state reset if this happens? How about accessing the instance and changing some things? This unfortunately wouldn't work. I think just making the user instantiate it would work fine.

NanderTGA commented 9 months ago

Fix has been released in msgroom@2.0.0-57.

NanderTGA commented 9 months ago

If this doesn't suffice, feel free to reopen.

msgroom-js-semantic-release[bot] commented 7 months ago

:tada: This issue has been resolved in version 2.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: