jeromeludmann / deno-irc

IRC client protocol module for Deno
https://deno.land/x/irc
MIT License
13 stars 4 forks source link

send cap messages all at once instead of piecemeal #7

Closed xyzshantaram closed 1 year ago

xyzshantaram commented 1 year ago

Okay, so, testing my bot on Libera from a DO server revealed that we still had the issue of getting blocked by Libera whenever we tried to connect to IRC. I'm guessing this is because we are sending caps piecemeal (introduced by yours truly in #6 because the protocol said it was fine to send multiple CAP END in a row). I have rewritten a bit of the registration / cap code to fix this. Opinions?

jeromeludmann commented 1 year ago

Looks more flexible now.

Does it work better on Libera with this change?

xyzshantaram commented 1 year ago

Actually, it's still broken. I'm going to do some tests...

xyzshantaram commented 1 year ago

Okay, yeah, that fixed it, we have to send the CAP messages first thing. CAP "pauses" connection stuff apparently, if we send NICK and USER first that "seals our fate", so to speak.

xyzshantaram commented 1 year ago

Eventually we will have to do CAP LS instead of blindly requesting SASL. Because right now there's a small edge case where if we do CAP REQ SASL and the server doesn't acknowledge it we get stuck in limbo waiting for the connection to time out. (I suppose alternatively we could listen for a CAP NAK from the server.

xyzshantaram commented 1 year ago

Also, unrelated, but we should probably limit CI to running on PRs and pushing to main, because otherwise it tries to run CI on my experimental topic branch commits which is less than ideal.

jeromeludmann commented 1 year ago

Also, unrelated, but we should probably limit CI to running on PRs and pushing to main, because otherwise it tries to run CI on my experimental topic branch commits which is less than ideal.

Good point, I will try to change that.

xyzshantaram commented 1 year ago

Next steps: 1) Do CAP LS before requesting specific caps. (This should simplify registration by a bit since we won't need to send CAPs two different ways to ensure a connection). 2) Fix CI (I'll get on that tonight, you can specify specific branches to run it on). 3) I want to add some code to assist debugging by letting us specify a logger to print out or save the raw bytes sent / received from the server. Frequently I found myself doing something like:

```ts
const client = new Client ({ /* ... */ });
const originalSend = client.send.bind(client);
client.send = (...args) => {
    console.log("SEND", args.join("|"));
}
```
We can listen for received messages with simply `client.on('raw')`, but imo it would be helpful to also be able to see what's being sent and in what order (basically, watch an entire session as it plays out). Thoughts?

4) Could you tell me about the housekeeping that needs doing for releases and stuff so you don't have to be stuck doing it all the time?

Will update with more as I think of it.

jeromeludmann commented 1 year ago
  1. I want to add some code to assist debugging by letting us specify a logger to print out or save the raw bytes sent / received from the server. Frequently I found myself doing something like:
    const client = new Client ({ /* ... */ });
    const originalSend = client.send.bind(client);
    client.send = (...args) => {
       console.log("SEND", args.join("|"));
    }

    We can listen for received messages with simply client.on('raw'), but imo it would be helpful to also be able to see what's being sent and in what order (basically, watch an entire session as it plays out). Thoughts?

Currently, there is a simple verbose option allowing to produce output:

const client = new Client ({ verbose: true });

(see plugins/verbose.ts)

This plugin produces outputs related to what is received/sent. I don't have enough overview to say if it's useful or not.

For now I think it provides too many data (too much noise) and lacks of fine tuning. Maybe if we can enable just raw outputs, and/or just events, etc. it would be more convenient to use?

Note that it is based to core/hooks.ts which provides some functions like beforeCall(fn, ...), afterCall(fn, ...)

jeromeludmann commented 1 year ago
  1. Could you tell me about the housekeeping that needs doing for releases and stuff so you don't have to be stuck doing it all the time?

The only thing to do is to git tag vX.X.X on the last commit and a git push origin branch --tags. When branch will be merge to main with a new tag, the package will be available from https://deno.land/x

The last thing is to create release on Github with the new added tag.

xyzshantaram commented 1 year ago

3) Ah, that's my bad! I hadn't seen verbose.ts or the hook system. I think verbose is almost perfect as it stands, it might be useful to make it a string option so we can get either "formatted" output for just generally testing what's going on or "raw" output showing the stream of ircmsgs being exchanged. Potentially we could also allow for replacing console.log or even letting the user format the message as it's printed.

jeromeludmann commented 1 year ago
  1. Ah, that's my bad! I hadn't seen verbose.ts or the hook system. I think verbose is almost perfect as it stands, it might be useful to make it a string option so we can get either "formatted" output for just generally testing what's going on or "raw" output showing the stream of ircmsgs being exchanged. Potentially we could also allow for replacing console.log or even letting the user format the message as it's printed.

@xyzshantaram String sounds good to me.

I also thought of array like verbose: ["raw", "events", ...] in order to combine log types, and avoiding "raw" "raw_and_events" "raw_and_events_and_commands" etc. pattern.

Currently, the verbose mode provides:

We could imagine a improvement of verbose with:

// type
verbose: Array<"raw" | "events" | "commands" | "state">

and it would be used like following:

// IRC message (raw) only
new Client({
  // ...
  verbose: ["raw"]
})

// Events and Commands (not raw)
new Client({
  // ...
  verbose: ["events", "commands"]
})

Note that I think values like:

could also be nice: easier to configure and probably also useful. 👍

xyzshantaram commented 1 year ago

I am not sure how useful allowing logging both raw and parsed messages at the same time would be. I think the use cases are primarily going to be either inspecting the raw traffic or the parsed messages and state changes. Also, it's a bit clunky to use an array for only a single log state, so maybe the type could be verbose?: "raw" | "parsed" | state" | Array<"raw" | "parsed" | "state">.

Alternatively,

new Client({
    logger: (msg: Command | Event | StateChange | Raw | string) => {
        if (msg instanceof Command) logMsg();
        /* snip */
    }
})

or something similar would add maximum flexibility by allowing to say write logs to a file or upload them somewhere, customizing the formatting, etc but the problem then is that it's up to the user to filter out their necessary log types. I suppose we could provide a few common loggers by default, so it would be

verbose?: ((msg: Command | Raw | Event | StateChange | string) => void) | "formatted" | "raw";

Where formatted uses a prewritten that allows inspecting the constructed message while debugging parsing/communication code and raw uses a prewritten logger that allows inspecting the bytes being sent to/from the server.

jeromeludmann commented 1 year ago

I am not sure how useful allowing logging both raw and parsed messages at the same time would be. I think the use cases are primarily going to be either inspecting the raw traffic or the parsed messages and state changes. Also, it's a bit clunky to use an array for only a single log state, so maybe the type could be verbose?: "raw" | "parsed" | state" | Array<"raw" | "parsed" | "state">.

You are right. "raw" or "advanced" seems more obvious.

Alternatively,

new Client({
    logger: (msg: Command | Event | StateChange | Raw | string) => {
        if (msg instanceof Command) logMsg();
        /* snip */
    }
})

Determining the type with instanceof seems tricky to me, but alternative could be to wrap messages inside an object containing a type:

type LogMsg = {
  type: 'received_raw' | 'sent_raw' | 'events' | 'commands'
  msg: MsgPayload
}

logger: (logMsg: LogMsg) => {
  switch (logMsg.type) {
    case 'received_raw': /* { type: 'received_raw', msg: MsgPayload } */
    case 'sent_raw': /* { type: 'sent_raw', msg: MsgPayload } */
    case 'events': /* { type: 'events', msg: MsgPayload } */
    case 'commands': /* { type: 'events', msg: MsgPayload } */
  }
}

or something similar would add maximum flexibility by allowing to say write logs to a file or upload them somewhere, customizing the formatting, etc but the problem then is that it's up to the user to filter out their necessary log types. I suppose we could provide a few common loggers by default, so it would be

verbose?: ((msg: Command | Raw | Event | StateChange | string) => void) | "formatted" | "raw";

Where formatted uses a prewritten that allows inspecting the constructed message while debugging parsing/communication code and raw uses a prewritten logger that allows inspecting the bytes being sent to/from the server.

Interesting. I could do a pre impl this week.

So raw or formatted would use a built in verbose function, and a third choice would be to implement yourself the verbose function.

xyzshantaram commented 1 year ago

Determining the type with instanceof seems tricky to me, but alternative could be to wrap messages inside an object containing a type:

Fair enough, that is pretty reasonable.

Interesting. I could do a pre impl this week.

If you have the time. I would love to help out as well.

So raw or formatted would use a built in verbose function, and a third choice would be to implement yourself the verbose function.

Yes, exactly.