Drop slate-irc, switch to irc-framework

[xPaw]

💅 This is rather huge, opening a PR for tracking and discussion.

What's left to do:

• Merge Make whois a client template #161 before fixing whois
• Merge Handle commands in a better way #154
• Invites are not handled, Handle invites kiwiirc/irc-framework#19
  • Confirm Invite message is not built correctly #121
• User list does not strip modes correctly Modes are not stripped from nicks in userlist kiwiirc/irc-framework#17
• User mode not updated in mode event
  • Does a NAMES request just like the current master solution, this will need to be tackled in a future PR
• Need to end the connection correctly (quit command) Document on how end the connection correctly kiwiirc/irc-framework#21
• The only test we have is broken
• from_server is not being set to true and causes server notices to be sent into a query window
• Channels need to be searched case insensitively
• Timestamps in topic set by need to be parsed
• Missing sender name in quakenet notices crashes Drop slate-irc, switch to irc-framework #167 (comment)

Thing to keep in mind: the framework keeps its own track of channels and users in them, we can use it to our advantage.

New features:

• IRCv3 compliance
  • SASL should work broken
  • server-time support on the client
  • Not per spec, but twitch.tv/membership is supported
• Displays who and when set the topic
• Connection messages (like connecting, reconnecting, etc) in server window, partial Show all messages from server #56 fix and Show connection attempt status messages in network buffer #115
• Server window is created as soon as possible to allow displaying errors (like disallowed hostname)
• Fixes Only the first nick is displayed when multiple people are Opped/Voiced #166 to correctly display each affected user on mode changes
• We will be able to support reconnections, however they are disabled for now as I don't know how well lounge will handle them
• Invite message is not built correctly #121 should be fixed by itself since irc-framework correctly parses different messages
• Channel names are no longer forcefully lower cased (slate did that)
• Target groups (STATUSMSG) is now handled (e.g. /msg +#freenode hello)

[lindskogen]

And we can write a middleware that persists messages to a sqlite database 👍

[xPaw]

Pinging @thelounge/maintainers to test this branch for any obvious breakage (besides the ones mentioned in op post)

[astorije]

@xPaw, nice work! I'll look at what I can simply add in terms of tests before I move forward.

[maxpoulin64]

Quick test so far, looks really great overall! Few things I noticed:

• When connecting to ZNC, all my past PMs are received from myself in one giant conversation. Probably has to do with the way messages are parsed?
• Topic timestamps are not parsed Topic set by Max-P on 1457043909
• All notices during connect are open as seperate windows. I new about the IRC server one, but it appears that NickServ and SaslServ also opens their own windows as well with their notices on connect. They probably should end up in the server window as well.

You probably know about those but I'm letting them here for tracking purposes.

[lpoujol]

Connecting to Quakenet crashes lounge.
Seems because Quakenet sends notice at connexion that contains no source.

telnet irc.quakenet.org 6667
Trying 128.39.65.226...
Connected to irc.quakenet.org.
Escape character is '^]'.
NOTICE AUTH :* Looking up your hostname
NOTICE AUTH :* Checking Ident
NOTICE AUTH :* Found your hostname
NOTICE AUTH :* No ident response

Here's lounge console with logs as xPaw asked when connecting to Quakenet :
http://sebsauvage.net/paste/?89c9e53662aac5e1#kgfis+2KKzmP66s4CL/gMeOwPcaxGSQwgsDutQWAXrQ=

[astorije]

@xPaw, just rebased this branch, don't forget to git fetch && git reset --hard origin/irc-framework before anything!

[astorije]

Tests know reflect the PR correctly. It does show a couple breaks, but I didn't fix the code itself. I can take a look later.
(Also removed a trailing whitespace in your last commit ^^)

[astorije]

I think it would be nicer to only pass the prefixes in the sortUsers function. To assist with that, we can have Network.prototype.prefixes = function() in network.js that returns the array of prefixes for that network. We can then call things like chan.sortUsers(network.prefixes()); in a simple way (simplify tests too).

I was trying to provide it, but I noticed there is options.PREFIX, serverOptions.PREFIX and prefixLookup, all related. It seems to me that sortUsers should be using serverOptions.PREFIX instead of options.PREFIX but I didn't look at the code too closely. It would be better to keep a single source of truth and use methods to produce the others.

Also, in places calling sortUsers(), there are irc.network and network. Do they carry the same values?

Happy to help on this.

[xPaw]

It seems to me that sortUsers should be using serverOptions.PREFIX instead of options.PREFIX

They're the same thing. serverOptions exists because its an object in our network instance, and it gets synchronized to the frontend client for later use. I didn't want to pipe the whole irc-fw options/networks objects to the client, as they have a lot of data.

[astorije]

I didn't want to pipe the whole irc-fw options/networks objects to the client, as they have a lot of data.

Yep, agreed!

However, I think those add some confusion to the codebase overall, I'm sure we can encapsulate this nicely in the models and interface that (options, serverOptions, ...) via methods. What do you think? I can try to spend some time on that if that's fine by you.

[xPaw]

We probably can change toJSON method in network to return whitelisted properties, like export does.

[astorije]

@xPaw, I'll test more extensively and add stuff over the weekend. Meanwhile, could you add @maxpoulin64 and @lpoujol's comments to your punch list above?

[xPaw]

I think this should have been fixed in the test, as irc-fw will only be providing us with modes array.

[astorije]

I added changes here simply because of the TODO above. If the model changes because irc-framework makes us too, I think we should go all in: name could be reworded nick everywhere and the non-array mode could disappear.
In the meantime, it was an easy way for me to integrate the tests, until this TODO gets addressed.
@xPaw, what's your take about changing the model regarding name/nick/mode/modes?

[xPaw]

It's not an easy change because it will affect a lot of files. I'd rather tackle it in a separate PR because this one is getting pretty big already.

[xPaw]

That's not how PREFIX is formatted by the way, which is why the test is failing. See https://github.com/kiwiirc/irc-framework/blob/48bb805653872e5f21210474fdfe439892be91b0/src/networkinfo.js#L12

[astorije]

I don't see how this behaviour is possibly different from what master does. Get nick in use -> append numbers.

Nah, I just double-checked, assuming I'm astorije:

• on v1.4.3: /nick xPaw fails with Nickname is already in use. and my nick remains astorije.
• on irc-framework branch: nick xPaw fails with xPaw: Nickname is already in use. but my nick gets changed to xPaw<n>.

If nick is already in use, command should be canceled altogether. I can take a look if you want :-)

All errors get sent into network lobby, because we never decided where to display them.

Sorry, should have been clearer: my point was that user_on_channel is directly displayed, instead of something like <nick>: User is already on channel.

What weird network is this on again? Works fine on freenode.

Actually, this is on Freenode: A is on v1.4.3, B is on irc-framework, A invites B to a channel where B isn't (but where A is), which leads to:

Weird...

Again, we never decided where errors should be going.

Somehow I can't reproduce this, so marking that as fixed, we'll deal with this later if needed.

[astorije]

Alright, I'm done testing I believe :-)
I have tested a few edge cases and very few and minor bugs discovered, that's great! I have reported all of them in comments, inline or not.

Only major issue I have noticed is that running the /list command has triggered 2 different types of errors, depending on the network (one of them being Freenode):

[...]/lounge/node_modules/irc-framework/src/commands/handlers/misc.js:12
        if (cache.channels.length) {
                          ^

TypeError: Cannot read property 'length' of undefined
    at IrcCommandHandler.handlers.RPL_LISTEND ([...]/lounge/node_modules/irc-framework/src/commands/handlers/misc.js:12:27)
    at IrcCommandHandler.dispatch ([...]/lounge/node_modules/irc-framework/src/commands/handler.js:49:37)
    at IrcCommandHandler._write ([...]/lounge/node_modules/irc-framework/src/commands/handler.js:35:10)
    at doWrite (_stream_writable.js:292:12)
    at writeOrBuffer (_stream_writable.js:278:5)
    at IrcCommandHandler.Writable.write (_stream_writable.js:207:11)
    at MiddlewareStream.ondata (_stream_readable.js:528:20)
    at emitOne (events.js:77:13)
    at MiddlewareStream.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)

and:

[...]/lounge/node_modules/irc-framework/src/commands/handlers/misc.js:23
        cache.channels.push({
                      ^

TypeError: Cannot read property 'push' of undefined
    at IrcCommandHandler.handlers.RPL_LIST ([...]/lounge/node_modules/irc-framework/src/commands/handlers/misc.js:23:23)
    at IrcCommandHandler.dispatch ([...]/lounge/node_modules/irc-framework/src/commands/handler.js:49:37)
    at IrcCommandHandler._write ([...]/lounge/node_modules/irc-framework/src/commands/handler.js:35:10)
    at doWrite (_stream_writable.js:292:12)
    at writeOrBuffer (_stream_writable.js:278:5)
    at IrcCommandHandler.Writable.write (_stream_writable.js:207:11)
    at MiddlewareStream.ondata (_stream_readable.js:528:20)
    at emitOne (events.js:77:13)
    at MiddlewareStream.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)

It's fairly inconsistent, and for the first one I sometimes had to run /list 2-3 times in a row to see it happen.
Note that these happen even with the code to handle /list absent from the PR :-(

It seems pretty critical to me, as it gets easy to kill an instance, like our demo. @xPaw, what do you recommend here? Is it reasonable to suggest a custom input plugin for /list that just does nothing until the feature appears for real? If so, I'm guessing the using a raw command would still fail and maybe we'd have to hardcode ignoring /list... Very much not ideal, so I'm open to suggestions, even short term ones to get this PR quickly merged into master.

---

So yeah, a few minor bugs, that big one, and then I think we can ship this PR :-)

[xPaw]

@astorije I suggest we take a good look at error messages in a future PR, see https://github.com/prawnsalad/KiwiIRC/blob/6f90124e0f15d4cc373496110d1150ffd75072fc/client/src/models/network.js#L856-L917

And I see about the random name now, it only added random numbers if you were not "connected" yet. Fixed

I don't see how you are possibly hitting a crash in /list, since the cache.channels array is always created in RPL_LISTSTART.

[astorije]

@astorije I suggest we take a good look at error messages in a future PR

Yep!

And I see about the random name now, it only added random numbers if you were not "connected" yet.** Fixed**

Nice, tested and works OK! Still not too fond of the i<something> string but good enough for this PR.

I don't see how you are possibly hitting a crash in /list, since the cache.channels array is always created in RPL_LISTSTART.

See it in action :-) On the left, it's Freenode, on the right, it's not. Each of them produces one of the 2 stacktraces I posted above:

[astorije]

@astorije: When a the current user receives an invite, the notification displays <nick> invited you to instead of <nick> invited you to <channel>
@xPaw: What weird network is this on again? Works fine on freenode.

I'm not sure how come you couldn't reproduce this as I found this to be actually a bug. I fixed it in af2c36e, one less to go :-)

[xPaw]

@astorije /list will be fixed in irc-fw it self, we will just need to update it. Are there any other concerns left or not? If not, I say we can merge it (but not release) and then update irc-fw separately.

The remaining points you have are all related to displaying error messages, which we agreed to take a look at in a future discussion.

[astorije]

Great re: /list bug!
I'll take a pass on the thread to see if things have been left out (apart from those we said we'd tackle later).
Let's wait a few days before merging so that @YaManicKill can get back to us after a bit of testing, and after that, definitely let's merge and delay a tiny bit the following release.

[AlMcKinlay]

I've been running it for the last few days and not noticed anything that
hasn't been brought up already. I say just go for it.

On Wed, 13 Apr 2016, 03:16 Jérémie Astori, notifications@github.com wrote:

Great re: /list bug!
I'll take a pass on the thread to see if things have been left out (apart
from those we said we'd tackle later).
Let's wait a few days before merging so that @YaManicKill
https://github.com/YaManicKill can get back to us after a bit of
testing, and after that, definitely let's merge and delay a tiny bit the
following release.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#167 (comment)

[astorije]

Alright, looks like all comments have been addressed, or discussed enough to ensure they'll be addressed when time comes, everyone is 👍 on that, let's do it!!

[MaxLeiter]

Awesome work everyone involved! Awesome PR!