SlackBot.send sending strings in reverse order.

pwolfert commented 7 years ago

[x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
[x] I've read and agree to the Code of Conduct.
[x] I've searched for any related issues and avoided creating a duplicate issue.

Description

SlackBot.send seems to be sending messages in reverse order.

Reproducible in:

hubot-slack version: 4.2.2 OS version(s): Mac OS 10.12.1 Device(s): MacBook Pro

Steps to reproduce:

These messages reach Slack in reverse order from message 4 to message 1:

robot.hear(/hey hey hey/i, (res) => {
    res.send('message 1', 'message 2', 'message 3', 'message 4');
});

These messages reach Slack from message 1 to message 4:

robot.hear(/hey hey hey/i, (res) => {
    res.send('message 4', 'message 3', 'message 2', 'message 1');
});

Expected result:

message 1 message 2 message 3 message 4

Actual result:

message 4 message 3 message 2 message 1

Looking at bot.cofee and client.coffee, the problem isn't obvious to me. I'm not sure why it does this. My personal solution right now is to always reverse them, but that messes with my automated tests, which don't use the Slack adapter and expect the messages to be in the right order.

coryallegory commented 7 years ago

+1

johnagan commented 7 years ago

hey hey hey

The initial clue to this seems to be the hubot-slack client's send method doesn't accept a splat of messages, but the hubot adapter does.

What's not clear to me is how you're even getting 4 messages. It seems to be getting passed to the node-slack-sdk as an array and maybe it's processed backwards there? ¯_(ツ)_/¯

aleksmark commented 7 years ago

I am facing the same issue after updating to hubot-slack@4.3.4.

The problem is not just the reverse order of the messages.Also, when the shell script is called from the coffee script, the messages comes out, to slack, of order.

The order is correct on hubot-slack@4.0.4 version

test.coffee script

path = require 'path'
fs = require 'fs'
{spawn} = require 'child_process'
recursive = require 'recursive-readdir'
http = require 'http'

module.exports = (robot) ->

    robot.respond /test cmd/i, (msg) ->

        msg.send "foo", "bar"
        msg.send "foo1", "bar1"

        execFile = path.resolve('test.sh')

        fs.access execFile, fs.R_OK, (err) ->
            ls = spawn execFile
            ls.stdout.on 'data', (data) ->
                msg.send data.toString().trim()
            ls.stderr.on 'data', (data) ->
                msg.send data.toString().trim()

test.sh script

#!/bin/bash

echo "in bash script 1"
echo "in bash script 2"

Output - hubot-slack@4.0.4 version

foo 
bar 
foo1 
bar1 
in bash script 1
in bash script 2

Output - hubot-slack@4.0.5 version and all version after 4.0.4 and up to latest (4.3.4)

bar1
in bash script 1
in bash script 2 
foo1 
bar 
foo

I guess it has something to do with this commit https://github.com/slackapi/hubot-slack/commit/0fa38c622270a8de09b00af15cc6ab0055421d3f

I am wondering Is there any way of fixing the order, rather than downgrading hubot-slack to 4.0.4 version ?

Edit

Problem still exists even on hubot-slack@4.0.4, if the shell script produced high rate output (In my case I am running docker build in the script)

chapmanc commented 7 years ago

I am seeing this as well. It's sending them in a pretty random order. Is there any plans to fix this?

chapmanc commented 7 years ago

Should be fixed with the above. Old version of priorityQueue was being used. Since no priority was passed I replaced it with a queue and it works now. Can cherry pick my changes if you want a quick fix. @aleksmark @johnagan @coryallegory @pwolfert

aoberoi commented 7 years ago

@chapmanc, thanks. i'll get out a release today!

chapmanc commented 7 years ago

@aoberoi Any chance of that release happening for hubot-slack and the node-slack-client? I am overriding for now but would prefer not to.

aoberoi commented 7 years ago

my apologies for the delay. we had some major changes landing in node-slack-sdk and i wanted to batch them all in the same release so i didn't end up making multiple releases in a week. i'm still hoping to land 2 more PRs in that project before the release, but i feel confident it will happen in the next couple days. again, sorry i didn't come back and update this issue as this stuff happened.

chapmanc commented 7 years ago

Not at all! Thanks for your work! It's much appreciated. @aoberoi